Improve internal opentelemetry logging #2128

lalitb · 2024-09-19T07:15:25Z

Fixes #1146

Changes

Created the draft PR to discuss the approach as mentioned here

Have added comment on some of the open issues.

Sample out of the appender-tracing example:

Scenario 1: only output internal error:

Scenario 2: output info and above:

Scenario 3: output 'debug' and above:

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)

codecov · 2024-09-19T07:19:00Z

Codecov Report

Attention: Patch coverage is 78.37838% with 8 lines in your changes missing coverage. Please review.

Project coverage is 78.3%. Comparing base (3976f3d) to head (e5260d9).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
opentelemetry-sdk/src/logs/log_emitter.rs	76.4%	4 Missing ⚠️
opentelemetry-sdk/src/logs/log_processor.rs	76.4%	4 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #2128   +/-   ##
=====================================
  Coverage   78.3%   78.3%           
=====================================
  Files        121     121           
  Lines      20767   20804   +37     
=====================================
+ Hits       16268   16300   +32     
- Misses      4499    4504    +5

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

…emetry-rust into log-handler-using-tracing

lalitb · 2024-09-19T07:58:28Z

opentelemetry-appender-tracing/Cargo.toml

@@ -12,16 +12,16 @@ rust-version = "1.65"

 [dependencies]
 log = { workspace = true, optional = true }
-opentelemetry = { version = "0.25", path = "../opentelemetry", features = ["logs"] }
+opentelemetry = { version = "0.25", path = "../opentelemetry", features = ["logs","experimental-internal-debugging"] }


name of the experimental flag - experimental-internal-debugging ?

lalitb · 2024-09-19T08:01:18Z

opentelemetry-appender-tracing/examples/basic.rs

+        .with_filter(filter_fn(|meta| meta.target().starts_with("opentelemetry"))) // Custom filter function
+        .with_filter(env_filter);
+    tracing_subscriber::registry()
+        .with(internal_log_layer)


as the internal_log_layer can be added only after the LoggerProvider is created, any internal logs till that time won't be logged. This would be the case for logs and traces, and not metrics.

lalitb · 2024-09-19T08:08:07Z

opentelemetry-appender-tracing/src/layer.rs

@@ -157,6 +157,9 @@ where
        event: &tracing::Event<'_>,
        _ctx: tracing_subscriber::layer::Context<'_, S>,
    ) {
+        if event.metadata().target().starts_with("opentelemetry") {
+            return;
+        }


To prevent infinite logging, events with the target prefix opentelemetry are suppressed at the on_event level. Suppressing at the enabled level would block these events from reaching other layers (both higher and lower in the order). While suppressing at the on_event level introduces a minimal overhead (~ .6% in stress test), this approach is safer as it ensures that if the application owner forgets to handle suppression, the application won't crash with a stack overflow. We need to discuss the best approach based on these considerations.

lalitb · 2024-09-19T08:12:45Z

opentelemetry/src/global/internal_logging.rs

+/// - `target`: The component or module generating the log. This identifies which crate or system part is logging the message (e.g., "opentelemetry-sdk", "opentelemetry-otlp").
+/// - `name`: The operation or action being logged. This provides context on what the system is doing at the time the log is emitted (e.g., "sdk_initialization", "exporter_start").
+/// - `signal`: The type of telemetry data related to the log. This could be "trace", "metric", or "log", representing the OpenTelemetry signal.
+///


The target, name, and signal fields are mandatory. While recommendations are provided for what these fields should contain, their values are not strictly enforced. Further discussion is needed to determine whether a more specific representation or stricter validation should be implemented for these fields.

cijothomas · 2024-09-19T19:31:30Z

opentelemetry-appender-tracing/examples/basic.rs

-    tracing_subscriber::registry().with(layer).init();
-
-    error!(name: "my-event-name", target: "my-system", event_id = 20, user_name = "otel", user_email = "[email protected]", message = "This is an example message");
+    let env_filter = EnvFilter::from_default_env();


this example is to show the usage of appender-tracing, and we can leave it untouched. We need to fix https://github.com/open-telemetry/opentelemetry-rust/tree/main/examples/self-diagnostics to showcase this, if needed.
I am unsure if we need to show this with a full blown example, but no harm doing it in https://github.com/open-telemetry/opentelemetry-rust/tree/main/examples/self-diagnostics

cijothomas · 2024-09-19T19:36:54Z

opentelemetry-sdk/src/logs/log_emitter.rs

@@ -49,6 +50,9 @@ impl opentelemetry::logs::LoggerProvider for LoggerProvider {
        attributes: Option<Vec<opentelemetry::KeyValue>>,
    ) -> Logger {
        let name = name.into();
+        otel_info!(target: "opentelemetry-sdk", name: "logger_versioned_creation", signal: "log", 


target
I think the usage of component name for target looks good. Though we need to discuss further about making it even more specific, like opentelemetry-sdk.log.logemitter?

name
Looks good. We need to develop conventions. (like using "." vs "_" separator).

signal
I don't think this is required. Target+Name should be sufficient.

message
I think we should only promote key/values pairs, and leave the message out, unless there is a strong need.
Lets also use a named field as that is proved much faster : #2001

cijothomas · 2024-09-20T01:56:14Z

opentelemetry/src/global/internal_logging.rs

@@ -0,0 +1,130 @@
+#![allow(unused_macros)]
+/// Macro for logging messages at the general log level in OpenTelemetry.


a warning at the top to indicate that this meant for user by OpenTelemetry components itself.

lalitb added 5 commits September 18, 2024 02:37

initial commit

7da284e

instrument internal code

573994d

instrument..

1bfd464

instrument..

90470c3

update example

5f1b990

lalitb requested a review from a team as a code owner September 19, 2024 07:15

Merge branch 'main' into log-handler-using-tracing

47abb6d

lalitb marked this pull request as draft September 19, 2024 07:16

lalitb added 2 commits September 19, 2024 00:43

update example

46ce8af

Merge branch 'log-handler-using-tracing' of github.com:lalitb/opentel…

bbfc03c

…emetry-rust into log-handler-using-tracing

lalitb commented Sep 19, 2024

View reviewed changes

lalitb mentioned this pull request Sep 19, 2024

Proposal to improve internal error logging #1146

Open

changes for filtering

e5260d9

cijothomas reviewed Sep 19, 2024

View reviewed changes

cijothomas reviewed Sep 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve internal opentelemetry logging #2128

Improve internal opentelemetry logging #2128

lalitb commented Sep 19, 2024 •

edited

Loading

codecov bot commented Sep 19, 2024 •

edited

Loading

lalitb Sep 19, 2024

lalitb Sep 19, 2024

lalitb Sep 19, 2024 •

edited

Loading

lalitb Sep 19, 2024

cijothomas Sep 19, 2024

cijothomas Sep 19, 2024

cijothomas Sep 20, 2024

		@@ -0,0 +1,130 @@
		#![allow(unused_macros)]
		/// Macro for logging messages at the general log level in OpenTelemetry.

Improve internal opentelemetry logging #2128

Are you sure you want to change the base?

Improve internal opentelemetry logging #2128

Conversation

lalitb commented Sep 19, 2024 • edited Loading

Changes

Merge requirement checklist

codecov bot commented Sep 19, 2024 • edited Loading

Codecov Report

lalitb Sep 19, 2024

Choose a reason for hiding this comment

lalitb Sep 19, 2024

Choose a reason for hiding this comment

lalitb Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

lalitb Sep 19, 2024

Choose a reason for hiding this comment

cijothomas Sep 19, 2024

Choose a reason for hiding this comment

cijothomas Sep 19, 2024

Choose a reason for hiding this comment

cijothomas Sep 20, 2024

Choose a reason for hiding this comment

lalitb commented Sep 19, 2024 •

edited

Loading

codecov bot commented Sep 19, 2024 •

edited

Loading

lalitb Sep 19, 2024 •

edited

Loading