-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[mdatagen] generate utility code to test telemetry #10212
[mdatagen] generate utility code to test telemetry #10212
Conversation
This will allow components to more easily test the telemetry they should be emitting. Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10212 +/- ##
=======================================
Coverage 92.53% 92.54%
=======================================
Files 387 387
Lines 18191 18198 +7
=======================================
+ Hits 16833 16841 +8
+ Misses 1019 1016 -3
- Partials 339 341 +2 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really nice! Just a couple of comments around a possible shutdown function, but LGTM otherwise.
"go.opentelemetry.io/collector/receiver/receivertest" | ||
) | ||
|
||
type componentTestTelemetry struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make sense to provide a "Reset" function here, so that multiple tests can exist without re-creating test settings for each case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to add this as needed. I suspect it will be needed sooner than later but this particular PR doesn't need it just yet
reader := sdkmetric.NewManualReader() | ||
return componentTestTelemetry{ | ||
reader: reader, | ||
meterProvider: sdkmetric.NewMeterProvider(sdkmetric.WithReader(reader)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think either the metric provider or the manual reader needs to be shutdown after the test. Perhaps store the mp somewhere, and provide a shutdown function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely would be good to have a Shutdown(context)
on the componentTestTelemetry struct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shutdown added and used in the samplereceiver test
cmd/mdatagen/internal/samplereceiver/generated_component_telemetry_test.go
Show resolved
Hide resolved
reader := sdkmetric.NewManualReader() | ||
return componentTestTelemetry{ | ||
reader: reader, | ||
meterProvider: sdkmetric.NewMeterProvider(sdkmetric.WithReader(reader)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely would be good to have a Shutdown(context)
on the componentTestTelemetry struct.
Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
This will allow components to more easily test the telemetry they should be emitting.