Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Distributor shim: add test verifying receiver works (including metrics) #4477

Merged
merged 4 commits into from
Dec 19, 2024

Conversation

yvrhdn
Copy link
Member

@yvrhdn yvrhdn commented Dec 19, 2024

What this PR does:

Adds a test that runs the OTel receiver shim and tries to write traces in various formats. It also checks the spans_received/spans_refused metrics are set correctly.

I have also refactored some code:

  • to properly test the metrics, pass in a prometheus.Registerer instead of using the global one
  • embed the noop.Meter and remove all empty methods

I'm adding this because updating OTel dependencies is breaking the metrics and we only have slow integration tests to catch this 😞

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Tests updated
  • Documentation added Not needed
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Copy link
Contributor

@javiermolinar javiermolinar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, have you tested that the metrics are still exposed, using the docker-compose example for instance?

_ metric.Float64ObservableUpDownCounter = Float64ObservableUpDownCounter{}
_ metric.Int64Observer = Int64Observer{}
_ metric.Float64Observer = Float64Observer{}
_ metric.MeterProvider = &MeterProvider{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know if we need the pointer? I think we can keep the value receivers, none of the methods modify the struct state

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I've pushed a new commit with just value receivers. Works just as well.

@yvrhdn
Copy link
Member Author

yvrhdn commented Dec 19, 2024

Looks good, have you tested that the metrics are still exposed, using the docker-compose example for instance?

Yep, running the docker-compose/local example. If I check /metrics they are there still:

Screenshot 2024-12-19 at 16 02 57

@yvrhdn yvrhdn enabled auto-merge (squash) December 19, 2024 15:06
@yvrhdn yvrhdn merged commit 2b00b4e into main Dec 19, 2024
17 checks passed
@yvrhdn yvrhdn deleted the y/add-shim-test branch December 19, 2024 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants