-
Notifications
You must be signed in to change notification settings - Fork 68
Add service::telemetry::logs::providers settings for internal logging setup #1795
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.
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
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1795 +/- ##
==========================================
+ Coverage 84.34% 84.36% +0.01%
==========================================
Files 496 496
Lines 144435 144547 +112
==========================================
+ Hits 121829 121942 +113
+ Misses 22072 22071 -1
Partials 534 534
🚀 New features to boost your workflow:
|
lquerel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| - ITS: Use the internal telemetry system. | ||
| - OpenTelemetry: Use the OpenTelemetry SDK. | ||
| - ConsoleDirect: Synchronously write to the console. | ||
| - ConsoleAsync: Synchronously write to the console. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - ConsoleAsync: Synchronously write to the console. | |
| - ConsoleAsync: Asynchronously write to the console. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] | ||
| pub struct LoggingProviders { | ||
| /// Provider mode for non-engine threads. This defines the global Tokio | ||
| /// `tracing` subscriber. Default is Unbuffered. Note that Buffered |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update to newer terminology instead of mentioning "Buffered" and "Unbuffered".
| fn test_validate_engine_otel_requires_global_otel() { | ||
| use ProviderMode::*; | ||
| // Engine OpenTelemetry without global OpenTelemetry fails | ||
| for global in [Noop, ITS, ConsoleDirect] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| for global in [Noop, ITS, ConsoleDirect] { | |
| for global in [Noop, ITS, ConsoleAsync, ConsoleDirect] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| } | ||
| } | ||
|
|
||
| fn default_global_provider() -> ProviderMode { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: These methods can be marked const.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
| fn default_global_provider() -> ProviderMode { | ||
| ProviderMode::ITS | ||
| } | ||
|
|
||
| fn default_engine_provider() -> ProviderMode { | ||
| ProviderMode::ITS | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are not consistent with the default config yaml shared in the self_tracing_architecture.md which has ConsoleAsync as the default.
service:
telemetry:
logs:
level: info
providers:
global: console_async
engine: console_async
internal: noopThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. In the long term I hope ITS becomes the default, but for now ConsoleAsync is safest.
| OpenTelemetry, | ||
|
|
||
| /// Asynchronous console logging. | ||
| /// The caller writes to a channel the same as ITS deliver, but bypasses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// The caller writes to a channel the same as ITS deliver, but bypasses | |
| /// The caller writes to a channel the same as ITS delivery, but bypasses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
| /// Returns true if this requires a LogsReporter channel for | ||
| /// asynchronous logging. | ||
| #[must_use] | ||
| pub fn needs_reporter(&self) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| pub fn needs_reporter(&self) -> bool { | |
| pub const fn needs_reporter(&self) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I think I don't fully understand why I should use const keywords.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's mainly to help the compiler apply optimizations. However, marking a method const doesn't guarantee a performance improvement in all cases. The most evident benefit of using const on the method is that it allows your method to be called from const expressions.
On thinking some more, I think we also have to keep in mind that for public methods removing const later would be a breaking change.
…d/internal_config_2
Part of #1771.
Part of #1736.
As documented in #1741.
Updates that document to match this change reflecting the prototype in #1771.Revised relative to #1771.
Adds LoggingProviders (choice of default logging provider for global, engine, and internal-telemetry threads).
Adds ProviderMode with names to select instrumentation behavior, with
itsreferring to internal telemetry system.Note: These settings are somehow not ideally placed. They belong also in the top-level settings, or with observed_state settings. However, since logging is configured with resource and level, which are part of the service::telemetry config area presently, we use that structure. After the bulk of #1736 is finished we can restructure.