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 54.16667% with 22 lines in your changes missing coverage. Please review.

Project coverage is 79.0%. Comparing base (86dd486) to head (3c4d20c).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
opentelemetry-sdk/src/logs/log_processor.rs	19.0%	17 Missing ⚠️
opentelemetry-sdk/src/logs/log_emitter.rs	60.0%	2 Missing ⚠️
opentelemetry-sdk/src/metrics/periodic_reader.rs	0.0%	2 Missing ⚠️
opentelemetry-otlp/src/metric.rs	0.0%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #2128     +/-   ##
=======================================
- Coverage   79.0%   79.0%   -0.1%     
=======================================
  Files        121     121             
  Lines      20859   20856      -3     
=======================================
- Hits       16490   16482      -8     
- Misses      4369    4374      +5

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

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

opentelemetry-appender-tracing/Cargo.toml

opentelemetry-appender-tracing/examples/basic.rs

opentelemetry-appender-tracing/src/layer.rs

opentelemetry/src/global/internal_logging.rs

opentelemetry-appender-tracing/examples/basic.rs

opentelemetry-sdk/src/logs/log_emitter.rs

opentelemetry/src/global/internal_logging.rs

opentelemetry-sdk/src/logs/log_emitter.rs

shaun-cox · 2024-09-23T17:00:00Z

opentelemetry/src/global/internal_logging.rs

@@ -0,0 +1,200 @@
+#![allow(unused_macros)]


Why expose this from the opentelemetry (API) package if it is seemingly only going to be used by SDK or implementation specific crates in the project? Won't changes to this stuff necessitate a possible semver change in opentelemetry API crate itself, unnecessarily?

I kept it at the level of custom error handler which is within API. I thought there could be the cases where we may want to used within API e.g., debugging Context implementation.

i am not sure if there are better ways to "hide" it. It should be public API, so it follows the same sem ver rules, and it can be used by OTel itself + any other components extending otel. For example, if someone authors an exporter, they can also use this.
We can doc it so user end up misusing it for any other logging!

Won't changes to this stuff necessitate a possible semver change in opentelemetry API crate itself, unnecessarily?

We should treat this API as just any other API, so it cannot make breaking changes. As long as we are doing a minimal API (super thin wrapper around tracing), we should be able to stabilize it by 1.0, and keep it non-breaking forever (or until 2.0 ships)

@shaun-cox The current macros are lightweight and closely resemble the tracing macros. Since not all opentelemetry-* crates depend on opentelemetry-sdk, exposing these macros from the API package appears to be the most suitable approach.

Let me know if you see any concerns here.

opentelemetry-appender-tracing/Cargo.toml

opentelemetry-sdk/src/logs/log_emitter.rs

opentelemetry/src/global/internal_logging.rs

cijothomas

The changes look good! Thank you!.

I'd suggest to removing adding logs throughout into its own separate PR. We should be doing it very carefully (irrespective of the level), so its best to start with adding error level ones for the failure scenarios (that can be used to remove the current global handler), and anything else on a case-by-case basis, few at a time. (I am trying to be conservative here, as using tracing is still a very new thing, so lets start small)

lalitb · 2024-10-03T18:34:18Z

I'd suggest to removing adding logs throughout into its own separate PR.

~~Yes, this PR doesn't add any new logs. Just updates the logs which were added by the previous PR directly using tracing.~~

Ah, it does add some. I can remove it, but then the diagnostics example won't have much to show.

utpilla · 2024-10-03T22:58:54Z

opentelemetry-sdk/src/logs/log_emitter.rs

        Builder::default()
    }

    pub(crate) fn log_processors(&self) -> &[Box<dyn LogProcessor>] {
+        otel_debug!(


It should be good enough to get the processor count information just once during LoggerProvider build. Is there any value in repeatedly logging it?

to unblock and merge this PR quickly, I think its best to remove all internal logging that are not error/warning, and we can selective bring them one by one in future PRs.

utpilla · 2024-10-03T23:02:45Z

opentelemetry-sdk/src/logs/log_emitter.rs

@@ -104,6 +128,9 @@ impl LoggerProvider {

    /// Shuts down this `LoggerProvider`
    pub fn shutdown(&self) -> LogResult<()> {
+        otel_info!(


This should be logged only in case of a successful shutdown? Only in this case:

if errs.is_empty() { Ok(()) }

utpilla · 2024-10-03T23:08:13Z

opentelemetry-sdk/src/logs/log_emitter.rs

@@ -221,6 +260,9 @@ impl Logger {
        instrumentation_lib: Arc<InstrumentationLibrary>,
        provider: LoggerProvider,
    ) -> Self {
+        otel_info!(
+            name: "logger_new"


We should probably add some identification information for the logger such as name, version etc.

lalitb · 2024-10-03T23:33:00Z

All the debug/info has been removed now.

utpilla · 2024-10-03T23:42:12Z

opentelemetry-sdk/src/logs/log_processor.rs

@@ -98,16 +99,12 @@ impl LogProcessor for SimpleLogProcessor {
    fn emit(&self, record: &mut LogRecord, instrumentation: &InstrumentationLibrary) {
        // noop after shutdown
        if self.is_shutdown.load(std::sync::atomic::Ordering::Relaxed) {
+            otel_warn!(


This could continue to be the hot path even after shutdown so it might not be a great idea to keep logging warnings for this. Let's a TODO to revisit this and decide if we should remove the logging here or make it less severe.

opentelemetry-sdk/src/metrics/internal/mod.rs

Co-authored-by: Utkarsh Umesan Pillai <[email protected]>

cijothomas · 2024-10-04T00:08:08Z

opentelemetry-appender-tracing/src/layer.rs

@@ -234,8 +246,7 @@ mod tests {
            .with_simple_exporter(exporter.clone())
            .build();

-        let layer = layer::OpenTelemetryTracingBridge::new(&logger_provider);
-        let subscriber = tracing_subscriber::registry().with(layer);
+        let subscriber = create_tracing_subscriber(exporter.clone(), &logger_provider);


why is exporter required to be passed?

Forgot to remove as part of updating the method. Will cleanup in subsequent PR.

cijothomas · 2024-10-04T00:08:32Z

opentelemetry-appender-tracing/src/layer.rs

@@ -513,7 +521,7 @@ mod tests {
            let span_id = cx.span().span_context().span_id();

            // logging is done inside span context.
-            log::error!("log from log crate");
+            log::error!(target: "my-system", "log from log crate");


do we need these changes?

Yes we can remove the target, will follow up in subsequent cleanups.

cijothomas · 2024-10-04T00:11:16Z

opentelemetry-sdk/src/metrics/internal/mod.rs

@@ -147,6 +147,9 @@ impl<AU: AtomicallyUpdate<T>, T: Number, O: Operation> ValueMap<AU, T, O> {
            O::update_tracker(&new_tracker, measurement, index);
            trackers.insert(STREAM_OVERFLOW_ATTRIBUTES.clone(), Arc::new(new_tracker));
            global::handle_error(MetricsError::Other("Warning: Maximum data points for metric stream exceeded. Entry added to overflow. Subsequent overflows to same metric until next collect will not be logged.".into()));
+            otel_warn!( name: "ValueMap.measure",


These are user actionable logs, so it should use a end user friendly name like "MetricCarinalityLimitHit"/similar,.
Good to follow up separately on all logs.

Co-authored-by: Zhongyang Wu <[email protected]> Co-authored-by: Utkarsh Umesan Pillai <[email protected]>

cschramm · 2024-11-15T00:05:58Z

I observed obscure build issues with opentelemetry_sdk 0.27.0 and traced them down to this code, which requires tracing 0.1.39 while the workspace specifies 0.1.0 as the minimum version. While that is unfortunate, at least the dependency should reflect it.

lalitb · 2024-11-15T00:28:45Z

I observed obscure build issues with opentelemetry_sdk 0.27.0 and traced them down to this code, which requires tracing 0.1.39 while the workspace specifies 0.1.0 as the minimum version. While that is unfortunate, at least the dependency should reflect it.

Can you provide more details, where do you see 0.1.39 dependency added?

cschramm · 2024-11-15T11:49:07Z

Sure. The code is using the name syntax of the tracing macros which was added in tokio-rs/tracing#2699 and released in 0.1.39. Thus, when you cargo update -p tracing --precise 0.1.38 (or to 0.1.37 if you like, as 0.1.38 got yanked), cargo check -p opentelemetry_sdk fails.

cijothomas · 2024-12-02T20:41:59Z

#2373 Opening a new issue to track and address this.

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

opentelemetry-appender-tracing/Cargo.toml Outdated Show resolved Hide resolved

lalitb commented Sep 19, 2024

View reviewed changes

opentelemetry-appender-tracing/examples/basic.rs Outdated Show resolved Hide resolved

lalitb commented Sep 19, 2024

View reviewed changes

opentelemetry-appender-tracing/src/layer.rs Outdated Show resolved Hide resolved

lalitb commented Sep 19, 2024

View reviewed changes

opentelemetry/src/global/internal_logging.rs Show resolved Hide resolved

lalitb mentioned this pull request Sep 19, 2024

Proposal to improve internal error logging #1146

Closed

changes for filtering

e5260d9

cijothomas reviewed Sep 19, 2024

View reviewed changes

opentelemetry-appender-tracing/examples/basic.rs Outdated Show resolved Hide resolved

cijothomas reviewed Sep 19, 2024

View reviewed changes

opentelemetry-sdk/src/logs/log_emitter.rs Outdated Show resolved Hide resolved

cijothomas reviewed Sep 20, 2024

View reviewed changes

opentelemetry/src/global/internal_logging.rs Outdated Show resolved Hide resolved

TommyCpp and others added 2 commits September 21, 2024 15:10

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

a79b636

use structured logging

197b12a

shaun-cox reviewed Sep 23, 2024

View reviewed changes

cijothomas reviewed Sep 23, 2024

View reviewed changes

opentelemetry-appender-tracing/Cargo.toml Outdated Show resolved Hide resolved

TommyCpp reviewed Sep 24, 2024

View reviewed changes

opentelemetry-sdk/src/logs/log_emitter.rs Outdated Show resolved Hide resolved

TommyCpp reviewed Sep 24, 2024

View reviewed changes

opentelemetry/src/global/internal_logging.rs Outdated Show resolved Hide resolved

lalitb added 4 commits September 27, 2024 13:27

review comments

91e56ea

update self diagnostic

fa7148e

allow internal events in pipeline

a78357c

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

a3fba36

lalitb added 2 commits October 3, 2024 10:42

fix lint

d248750

lint

f9e3d2d

cijothomas reviewed Oct 3, 2024

View reviewed changes

use eprintln for warn and error if internal-logs flag is disabled

d4c1eb7

lalitb added 2 commits October 3, 2024 13:24

review comments

d5120a2

remove tracing from opentelemetry crate, and use name as metadata

c103467

utpilla reviewed Oct 3, 2024

View reviewed changes

lalitb added 3 commits October 3, 2024 16:20

fix

b7470b1

remove tracing from otel dev dep

30f7a4c

remove debug

adc3fd2

utpilla reviewed Oct 3, 2024

View reviewed changes

opentelemetry-sdk/src/metrics/internal/mod.rs Outdated Show resolved Hide resolved

utpilla approved these changes Oct 3, 2024

View reviewed changes

Update opentelemetry-sdk/src/metrics/internal/mod.rs

3c4d20c

Co-authored-by: Utkarsh Umesan Pillai <[email protected]>

cijothomas reviewed Oct 4, 2024

View reviewed changes

cijothomas approved these changes Oct 4, 2024

View reviewed changes

cijothomas merged commit 2ff7ec0 into open-telemetry:main Oct 4, 2024
24 of 25 checks passed

lalitb mentioned this pull request Oct 4, 2024

Remove global error handler in favor of internal logs #2175

Closed

cijothomas pushed a commit to cijothomas/opentelemetry-rust that referenced this pull request Oct 4, 2024

Improve internal opentelemetry logging (open-telemetry#2128)

9eaba05

Co-authored-by: Zhongyang Wu <[email protected]> Co-authored-by: Utkarsh Umesan Pillai <[email protected]>

cijothomas mentioned this pull request Dec 2, 2024

tracing minimum version dependency #2373

Open

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

shaun-cox Sep 23, 2024

lalitb Sep 23, 2024

cijothomas Sep 23, 2024 •

edited

Loading

lalitb Oct 2, 2024

cijothomas left a comment

lalitb commented Oct 3, 2024 •

edited

Loading

utpilla Oct 3, 2024

cijothomas Oct 3, 2024

utpilla Oct 3, 2024

utpilla Oct 3, 2024

lalitb commented Oct 3, 2024

utpilla Oct 3, 2024 •

edited

Loading

cijothomas Oct 4, 2024

lalitb Oct 4, 2024

cijothomas Oct 4, 2024

lalitb Oct 4, 2024

cijothomas Oct 4, 2024

cschramm commented Nov 15, 2024

lalitb commented Nov 15, 2024

cschramm commented Nov 15, 2024

cijothomas commented Dec 2, 2024

Improve internal opentelemetry logging #2128

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cijothomas Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cijothomas left a comment

Choose a reason for hiding this comment

lalitb commented Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lalitb commented Oct 3, 2024

utpilla Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cschramm commented Nov 15, 2024

lalitb commented Nov 15, 2024

cschramm commented Nov 15, 2024

cijothomas commented Dec 2, 2024

lalitb commented Sep 19, 2024 •

edited

Loading

codecov bot commented Sep 19, 2024 •

edited

Loading

cijothomas Sep 23, 2024 •

edited

Loading

lalitb commented Oct 3, 2024 •

edited

Loading

utpilla Oct 3, 2024 •

edited

Loading