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

[WIP] Suppress nested traces from instrumented dependencies #1315

Closed
wants to merge 5 commits into from

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented Oct 24, 2023

Fixes #1171

Need few changes before making it ready for review.

  • The SuppressionGuard struct should be runtime agnostic. As of now, it is using task-local from tokio runtime.
  • Need to add suppression logic across traces and logs, and OTLP-gRPC and OTLP-HTTP exporters.
    Future change : As of now, Context uses thread-local storage. It can also optionally support task-local storage as supported by otel-js, otel-dotnet. And then Context can be leveraged for suppression.

Changes

Please provide a brief description of the changes here.

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

@lalitb lalitb requested a review from a team October 24, 2023 06:00
@lalitb lalitb marked this pull request as draft October 24, 2023 06:00
@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

Attention: 84 lines in your changes are missing coverage. Please review.

Files Coverage Δ
opentelemetry-sdk/src/trace/mod.rs 100.0% <100.0%> (ø)
opentelemetry-sdk/src/logs/log_processor.rs 0.0% <0.0%> (ø)
opentelemetry-otlp/src/exporter/http/logs.rs 0.0% <0.0%> (ø)
opentelemetry-sdk/src/suppression.rs 0.0% <0.0%> (ø)

... and 27 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.


use super::OtlpHttpClient;

#[async_trait]
impl LogExporter for OtlpHttpClient {
async fn export(&mut self, batch: Vec<LogData>) -> LogResult<()> {
let _guard = SuppressionGuard::new();
Copy link
Member

Choose a reason for hiding this comment

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

hm holding this type of guard across await points does not generally work as expected. You would likely need to do a similar future wrapping strategy that we have in

impl<T: std::future::Future> std::future::Future for WithContext<T> {
type Output = T::Output;
fn poll(self: Pin<&mut Self>, task_cx: &mut TaskContext<'_>) -> Poll<Self::Output> {
let this = self.project();
let _guard = this.otel_cx.clone().attach();
this.inner.poll(task_cx)
}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion @jtescher. I tried the approach you suggested, basically holding the guard within Context, and try to propagate it across async functions. Seems I am still doing something different, as it is not working. Though I will debug if further, just in case you find some issue in the approach, or any other suggestions. I initially thought it would be straightforward and tokio task_local value is propagated automatically through the chain of async method calls, but probably this is not :)

@lalitb
Copy link
Member Author

lalitb commented Nov 1, 2023

closing this PR as @shaun-cox and @TommyCpp had alternate suggestion to extend the Context structure to support suppressing nested logs, Will be raising draft PR to further discuss that approach,

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.

Need a way to suppress telemetry from SDKs own operation
2 participants