[WIP] Suppression of nested logs from dependencies

[lalitb]

Raising this PR as alternate approach to fix #1171, by extending the Context structure to store the suppression flag. The PR is to discuss this approach. The earlier approach #1315 was based on tokio runtime, and generalizing it would have been difficult.

Change summary

1. Move FutureExt and WithContext constructs from trace/context.rs to context.rs.
2. Add suppress_logs flag in Context struct

pub struct Context {
    #[cfg(feature = "trace")]
    pub(super) span: Option<Arc<SynchronizedSpan>>,
    pub suppress_logs: bool,
    entries: HashMap<TypeId, Arc<dyn Any + Sync + Send>, BuildHasherDefault<IdHasher>>,
}

3. Add with_suppression method in Context to enable suppression:

impl Context {
  ...
    pub(super) fn with_suppression(&self) -> Self {
        Context {
            suppress_logs: true,
            entries: self.entries.clone(),
            ..self.clone()
        }
    }
}

4. Add with_current_context_supp method in FutureExt:

pub trait FutureExt: Sized {
    ...
    fn with_current_context_supp(self) -> WithContext<Self> {
        let otel_cx = Context::current().with_suppression();
        self.with_context(otel_cx)
    }
}

5. Suppress in Exporter::export method:

impl LogExporter for OtlpHttpClient {
    async fn export(&mut self, batch: Vec<LogData>) -> LogResult<()> {
        async {
            // export logic
       }
        .with_current_context_supp()
        .await
    }

6. Check if suppression is enabled in LogProcessor:

impl<R: RuntimeChannel<BatchMessage>> LogProcessor for BatchLogProcessor<R> {
    ...
    #[cfg(feature = "logs_level_enabled")]
    fn event_enabled(&self, _level: Severity, _target: &str, _name: &str) -> bool {
        !Context::current().suppress_logs
    }
}

7. How to test:

run opentelemetry-otlp/examples/basic-otlp-http/ example, while collector is running (using docker-compose up).

[cijothomas]

we should not tie this to the logs_level_enabled feature.

[lalitb]

Thanks for the comment. I believe placing it within logs_level_enabled is preferable for early suppression detection, which would avoid the need to generate a LogRecord. I also plan to add this validation in Logger::emit() for instances when the logs_level_enabled feature is not enabled, will be doing it before making it ready for review.

[cijothomas]

Agree doing it at the earliest possible place is perf friendly!

Can we do fn event_enabled always (i.e even without logs_level_enabled feature) and check Suppression inside that? And if logs_level_enabled feature is enabled, then do the additional check?

[lalitb]

Yes we can do that, adding this validation within appender for log and tracing, or better in LogRecord->Emit() or similar method once have implemented #1189. As of now, I am still bit struggling with propagating the future state across threads used by async task, so will revisit that once it is covered.

[cijothomas]

You are experimenting with logs, but the final expectation is that this will be applicable to traces and logs? If so, the name can be suppress_instrumentation

[lalitb]

Good observation. Got similar suggestion from @shaun-cox. I changed it to suppression, but probably suppress_instrumentation would be more descriptive.

[lalitb]

As this PR came into discussion in today's community meeting, summarising the challenge with the current solution -

The PR adds the suppress_logs flag in the Context, and the Context object is propagated inside the async task. However, wrapping the future with FutureExt::WithExt will not cascade the wrapping behavior to all internal futures. So if these internal futures (which could be from external (say) Hyper crate) get invoked in a separate thread, they won't see the suppression flag from context. And we can't explicitly apply the wrapper to each async function of third-party crates. This means, that if the logging macro is invoked from inside one of these async tasks, the suppression_flag won't be set and thus the logs are not suppressed.

[lalitb]

closing this PR. The changes would be done based on the options discussed here - #761 (comment).