Skip to content

Conversation

@mbrandenburger
Copy link
Member

@mbrandenburger mbrandenburger commented Aug 4, 2025

This PR addresses the issue reported in #1010

  • fix hardcoded debug log level
  • add log level check for expensive logging parms, such as, debug.Stack or base64 encoding
  • fix incorrect log caller for context based loggers

@mbrandenburger mbrandenburger added this to the Q3 milestone Aug 4, 2025
@mbrandenburger mbrandenburger requested a review from adecaro August 4, 2025 12:46
@mbrandenburger mbrandenburger self-assigned this Aug 4, 2025
@adecaro
Copy link
Contributor

adecaro commented Aug 4, 2025

Indeed, there was an issue there with the logging but now with this change to have the traces we need to set the logger level to debug. The side effect is that we will have lots of output on the terminal.

Wondering if we can have a way to set a different level for Otel alone.

otelzap.WithLoggerProvider(newLoggerProvider(zapLogger.Name())),
//otelzap.WithMinLevel(zapLogger.Level()),
otelzap.WithMinLevel(zapcore.DebugLevel),
otelzap.WithMinLevel(zapLogger.Level()),
Copy link
Contributor

Choose a reason for hiding this comment

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

we could use the already existing funciton

func Init(c Config) {
flogging.Init(c)
}

to allow a further customization for the otel level. So that even if we have info for the terminal, we have debug for otel.

Copy link
Member Author

Choose a reason for hiding this comment

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

as discussed offline, to keep configuration simple, we use our logging level for normal logs and "trace" event/logs. Introducing yet another customization for having different log levels for the logger and tracer may make the system more complicated.

Generally, we should use the tracing system to "track" the lifecycle of requests through the system and learn the interaction between multiple components and their execution latency. Tracing events/logs should give additional information if necessary, to better understand certain component execution flows. For details debugging, the component log should be used.
That is, we should aim to not overuse trace events/logs within spans, to maintain good readability and reduce additional performance overhead induced by tracing.

Copy link
Member Author

Choose a reason for hiding this comment

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

as discussed offline, to keep configuration simple, we use our logging level for normal logs and "trace" event/logs. Introducing yet another customization for having different log levels for the logger and tracer may make the system more complicated.

Generally, we should use the tracing system to "track" the lifecycle of requests through the system and learn the interaction between multiple components and their execution latency. Tracing events/logs should give additional information if necessary, to better understand certain component execution flows. For details debugging, the component log should be used. That is, we should aim to not overuse trace events/logs within spans, to maintain good readability and reduce additional performance overhead induced by tracing.

If we later feel, we would need to manage different logger/tracer logging-levels, we can introduce that feature later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with the not overusing bit. If I understand correctly, all logs via ..fContext go through this logger (+tracer)? Then this PR will at least make it a lot quieter (unless explicitly activated). Can we still add trace information to spans without also logging?

Copy link
Member Author

@mbrandenburger mbrandenburger Aug 27, 2025

Choose a reason for hiding this comment

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

Can we still add trace information to spans without also logging?

yes - you just need an instance of a span and then you create an event.

Example:

somespan.AddEvent("Read response")

@mbrandenburger mbrandenburger marked this pull request as draft August 5, 2025 08:54
@mbrandenburger
Copy link
Member Author

Indeed, there was an issue there with the logging but now with this change to have the traces we need to set the logger level to debug. The side effect is that we will have lots of output on the terminal.

I was thinking that the "side effect" is the expected behavior. If one runs in debug mode, you see more details as normal. That would include logs and traces. The current behavior, however, is that even if you turn logging off, debug traces are captured.

Wondering if we can have a way to set a different level for Otel alone.

We could have this as a new feature.

- fix hardcoded debug log level
- add log level check for expensive logging parms, such as, debug.Stack
- fix incorecct log caller for context based loggers

Signed-off-by: Marcus Brandenburger <bur@zurich.ibm.com>
@mbrandenburger mbrandenburger marked this pull request as ready for review August 26, 2025 11:32
Copy link
Contributor

@arner arner left a comment

Choose a reason for hiding this comment

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

LGTM!

@mbrandenburger mbrandenburger merged commit 8802fff into hyperledger-labs:main Aug 27, 2025
18 checks passed
@mbrandenburger mbrandenburger deleted the otel-loglevel branch August 27, 2025 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants