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

Consistent application of exporter customizers when otel.{signal}.exporter=none #7017

Conversation

jack-berg
Copy link
Member

While working on something else, I noticed a discrepancy between how autoconfigure handles exporter customization via SPI when otel.metrics.exporter=none, compared to otel.traces.exporter=none and otel.logs.exporter=none.

For traces and logs, when exporter is set to none, we still invoke any respective exporter customizers registered via AutoConfigurationCustomizer#addSpanExporterCustomizer, AutoConfigurationCustomizer#addLogRecordExporterCustomizer. For metrics, we do not apply such a AutoConfigurationCustomizer#addMetricExporterCustomizer when exporter is set to none.

This PR standardizes on the the metrics approach of not applying exporter customizers when exporter is set to none.

While you can make arguments for either approach, the metrics approach is more correct / defensible since its less surprising. I don't think many people would intuit that exporter customizers have a chance to intervene when the user intends to set the exporter to none.

@jack-berg jack-berg requested a review from a team as a code owner January 14, 2025 18:49
@@ -78,7 +78,6 @@ testing {
environment("OTEL_PROPAGATORS", "tracecontext,baggage,b3,b3multi,jaeger,ottrace,test")
environment("OTEL_EXPORTER_OTLP_HEADERS", "cat=meow,dog=bark")
environment("OTEL_EXPORTER_OTLP_TIMEOUT", "5000")
environment("OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT", "2")
Copy link
Member Author

Choose a reason for hiding this comment

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

This was interfering with the changes I was making to FullConfigTest. Setting span limits is well tested elsewhere, and we should limit the integration-style FullConfigTest to concepts that really require everything to be stood up in an end-to-end fashion.

Copy link

codecov bot commented Jan 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.98%. Comparing base (4b3cedd) to head (6dfbb06).
Report is 5 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7017      +/-   ##
============================================
+ Coverage     89.95%   89.98%   +0.02%     
+ Complexity     6636     6635       -1     
============================================
  Files           745      745              
  Lines         20010    20000      -10     
  Branches       1962     1960       -2     
============================================
- Hits          18000    17996       -4     
+ Misses         1415     1408       -7     
- Partials        595      596       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@breedx-splk breedx-splk changed the title Consistent application of exporter customizers when otel.{signal}.expoorter=none Consistent application of exporter customizers when otel.{signal}.exporter=none Jan 15, 2025
Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

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

Thanks for noticing and fixing the inconsistency! It would definitely be surprising to someone somewhere if they specified otel.{signal}.exporter=none and some customizer injected an exporter anyway. And I agree with your assessment that you could argue for/against this approach, but I think this way is the most practical/sensical.

@jack-berg jack-berg merged commit a1c0d0b into open-telemetry:main Jan 16, 2025
25 checks passed
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