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

suppress instrumentation: move to api + generic context key #6546

Merged
merged 7 commits into from
Jul 17, 2024

Conversation

SylvainJuge
Copy link
Contributor

@SylvainJuge SylvainJuge commented Jul 2, 2024

With #5918, the suppress_internal_exporter_instrumentation context key was introduced as a way to help identify the HTTP calls made by the OTLP exporter from the ones made by the application.

The name of the context key and the location of the related InstrumentationUtil class in the exporter make it exporter-only, which somehow blocks reusing it more broadly.

Other implementations seem to favor a more generic naming, so it seems reasonable to change:

  • python: suppress_instrumentation and suppress_http_instrumentation (definitions)
  • C# : otel.suppress_instrumentation (definition)

My goal here is to help unblock this PR open-telemetry/opentelemetry-java-contrib#1061 : okhttp is implicitly ignored for now due to classloader, but switching to internal JDK HTTP client means we need to have explicit exclusion

Also, this should help ensure that other internal HTTP clients usages like GCP/AWS (and maybe soon Azure too) cloud resource attributes providers are consistently excluded from tracing.

There has already been an issue opened to attempt adding this officially to the spec/semantic conventions in the past (open-telemetry/opentelemetry-specification#530) but there hasn't been any progress in a while, so we can conclude that there isn't any strong need to align this across platforms for now, this could however be a welcome later improvement.

@SylvainJuge SylvainJuge requested a review from a team July 2, 2024 09:04
*/
@Deprecated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[for reviewer] keeping this class allows to keep instrumentation agent compatible as it currently relies on it, switching to the new class location is the next step in the instrumentation agent once this has been merged and released.

Copy link
Contributor

@LikeTheSalad LikeTheSalad left a comment

Choose a reason for hiding this comment

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

Moving the tool outside of the exporters module should also help avoid using reflection here!

Copy link

codecov bot commented Jul 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.61%. Comparing base (20bcd75) to head (5b82090).

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6546      +/-   ##
============================================
+ Coverage     90.59%   90.61%   +0.01%     
- Complexity     6255     6257       +2     
============================================
  Files           689      690       +1     
  Lines         18698    18701       +3     
  Branches       1843     1843              
============================================
+ Hits          16940    16945       +5     
  Misses         1201     1201              
+ Partials        557      555       -2     

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

Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

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

Just the one comment on location, but I support this.

@SylvainJuge SylvainJuge changed the title suppress instrumentation: move to context + generic context key suppress instrumentation: move to api + generic context key Jul 12, 2024
@driverpt
Copy link

any ETA on this one ?

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.

4 participants