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.

Sign up for GitHub

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
Closed

[WIP] Suppress nested traces from instrumented dependencies #1315

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8f4b0eb
draft
lalitb Oct 23, 2023
bba8c03
use task local
lalitb Oct 24, 2023
f98bf85
fix
lalitb Oct 24, 2023
a65650c
review comment
lalitb Oct 30, 2023
bc9da9b
use task-local storage
lalitb Oct 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion opentelemetry-otlp/src/exporter/http/logs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,18 @@
use async_trait::async_trait;
use http::{header::CONTENT_TYPE, Method};
use opentelemetry::logs::{LogError, LogResult};
use opentelemetry_sdk::export::logs::{LogData, LogExporter};
use opentelemetry_sdk::{
export::logs::{LogData, LogExporter},
suppression::SuppressionGuard,
};
//use opentelemetry_sdk::suppression::Supression;

use super::OtlpHttpClient;

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

Check warning on line 17 in opentelemetry-otlp/src/exporter/http/logs.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-otlp/src/exporter/http/logs.rs#L17 

Added line #L17 was not covered by tests
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

opentelemetry-rust/opentelemetry/src/trace/context.rs

Lines 375 to 384 in 28d8cf9

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 :)

let client = self
.client
.lock()
Expand Down
2 changes: 2 additions & 0 deletions opentelemetry-sdk/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,8 @@ pub mod metrics;
pub mod propagation;
pub mod resource;
pub mod runtime;
/// doc
pub mod suppression;
#[cfg(any(feature = "testing", test))]
#[doc(hidden)]
pub mod testing;
Expand Down
5 changes: 3 additions & 2 deletions opentelemetry-sdk/src/logs/log_processor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crate::{
export::logs::{ExportResult, LogData, LogExporter},
runtime::{RuntimeChannel, TrySend},
suppression::SuppressionGuard,
};
use futures_channel::oneshot;
use futures_util::{
Expand Down Expand Up @@ -103,7 +104,7 @@

#[cfg(feature = "logs_level_enabled")]
fn event_enabled(&self, _level: Severity, _target: &str, _name: &str) -> bool {
true
!SuppressionGuard::is_logging_suppressed()

Check warning on line 107 in opentelemetry-sdk/src/logs/log_processor.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-sdk/src/logs/log_processor.rs#L107 

Added line #L107 was not covered by tests
}
}

Expand Down Expand Up @@ -132,7 +133,7 @@

#[cfg(feature = "logs_level_enabled")]
fn event_enabled(&self, _level: Severity, _target: &str, _name: &str) -> bool {
true
!SuppressionGuard::is_logging_suppressed()

Check warning on line 136 in opentelemetry-sdk/src/logs/log_processor.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-sdk/src/logs/log_processor.rs#L136 

Added line #L136 was not covered by tests
}

fn force_flush(&self) -> LogResult<()> {
Expand Down
31 changes: 31 additions & 0 deletions opentelemetry-sdk/src/suppression.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
use tokio::task_local;

// Define the async local storage for suppression.
task_local! {
static SUPPRESSION_FLAG: bool;
}

Check warning on line 6 in opentelemetry-sdk/src/suppression.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-sdk/src/suppression.rs#L4-L6 

Added lines #L4 - L6 were not covered by tests

/// Represents a scope within which logging is suppressed.
/// Logging is suppressed for the duration of the guard's lifetime.
#[derive(Debug)]

Check warning on line 10 in opentelemetry-sdk/src/suppression.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-sdk/src/suppression.rs#L10 

Added line #L10 was not covered by tests
pub struct SuppressionGuard(bool); // Capture the original state

impl SuppressionGuard {
/// doc1
pub fn new() -> Self {

Check failure on line 15 in opentelemetry-sdk/src/suppression.rs

View workflow job for this annotation

GitHub Actions / lint 

you should consider adding a `Default` implementation for `SuppressionGuard`
let original_state = SUPPRESSION_FLAG.try_with(|&flag| flag).unwrap_or(false);
SUPPRESSION_FLAG.scope(true, async {});
SuppressionGuard(original_state)
}

Check warning on line 19 in opentelemetry-sdk/src/suppression.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-sdk/src/suppression.rs#L15-L19 

Added lines #L15 - L19 were not covered by tests

/// doc2
pub fn is_logging_suppressed() -> bool {
SUPPRESSION_FLAG.try_with(|&flag| flag).unwrap_or(false)
}

Check warning on line 24 in opentelemetry-sdk/src/suppression.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-sdk/src/suppression.rs#L22-L24 

Added lines #L22 - L24 were not covered by tests
}

impl Drop for SuppressionGuard {
fn drop(&mut self) {
SUPPRESSION_FLAG.scope(self.0, async {}); // Restore the original state
}

Check warning on line 30 in opentelemetry-sdk/src/suppression.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-sdk/src/suppression.rs#L28-L30 

Added lines #L28 - L30 were not covered by tests
}
Loading