Skip to content

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

Merged
yvrhdn merged 4 commits intomainfrom
y/add-shim-test
Dec 19, 2024
Merged

Distributor shim: add test verifying receiver works (including metrics)#4477
yvrhdn merged 4 commits intomainfrom
y/add-shim-test

Conversation

@yvrhdn
Copy link
Copy Markdown
Contributor

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor 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
Copy Markdown
Contributor 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
@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