Internal logging provider setup; console_async support #1841
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…d/internal_logging_7
…d/internal_logging_7
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1841 +/- ##
==========================================
+ Coverage 84.64% 84.71% +0.07%
==========================================
Files 502 503 +1
Lines 148560 149956 +1396
==========================================
+ Hits 125749 127041 +1292
- Misses 22277 22381 +104
Partials 534 534
🚀 New features to boost your workflow:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
jmacd
commented
Jan 20, 2026
Contributor
Author
jmacd
left a comment
jmacd
left a comment
There was a problem hiding this comment.
A typical startup log,
2026-01-20T20:09:00.592Z INFO otap-df-otap::receiver.start (crates/otap/src/fake_data_generator.rs:131): [signals_per_second=100 signals/sec, max_batch_size=100, metrics_per_iteration=0, traces_per_iteration=0, logs_per_iteration=100]
cijothomas
reviewed
Jan 20, 2026
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jan 20, 2026
…#1843) Part of #1771. Part of #1736. Overlaps with #1841 by copying the file crates/telemetry/src/internal_events.rs to extend the otel_xxx macros to full Tokio syntax, to replace uses of log formatting as needed. After this, #1841 can remove "log" from the workspace Cargo.toml b/c crates/state will have the remaining "log" references fixed there.
utpilla
reviewed
Jan 21, 2026
| } | ||
|
|
||
| fn observe(&self, event: ObservedEvent) { | ||
| let sent = self.sender.send_timeout(event, self.timeout); |
Contributor
There was a problem hiding this comment.
If the channel is full, this would block the thread until the channel has space or the timeout expires? Since this would be called by the engine threads, do we want a non-blocking behavior instead?
Contributor
There was a problem hiding this comment.
I had also paused on this point. In addition to what I mentioned in another comment, I think that 1) we should first use try_send, and if that fails, we should then consult a policy, for example an enum with variants like timeout, drop, or block, and take the appropriate action based on that policy.
Contributor
Author
There was a problem hiding this comment.
Added a SendPolicy
/// How to act when an asynchronous event can't be sent.
#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)]
pub struct SendPolicy {
/// If set, wait for a timeout.
pub blocking_timeout: Option<Duration>,
/// If failed, issue a raw error to the console.
pub console_fallback: bool,
}
and let the existing engine events block / console log, while ordinary logging events will not block and drop.
lalitb
reviewed
Jan 21, 2026
utpilla
reviewed
Jan 21, 2026
utpilla
reviewed
Jan 21, 2026
lalitb
reviewed
Jan 21, 2026
lquerel
approved these changes
Jan 21, 2026
Contributor
lquerel
left a comment
lquerel
left a comment
There was a problem hiding this comment.
I think there are a few adjustments to make, but overall I'm fine with it. If my comments aren't clear enough, contact me on Slack.
Thanks for the update on the documentation.
Co-authored-by: Lalit Kumar Bhasin <lalit_fin@yahoo.com>
jmacd
commented
Jan 21, 2026
Comment on lines
+23
to
+47
| /// How to act when an asynchronous event can't be sent. | ||
| #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] | ||
| pub struct SendPolicy { | ||
| /// If set, wait for a timeout. | ||
| pub blocking_timeout: Option<Duration>, | ||
|
|
||
| /// If failed, issue a raw error to the console. | ||
| pub console_fallback: bool, | ||
| } | ||
|
|
||
| impl Default for ObservedStateSettings { | ||
| fn default() -> Self { | ||
| Self { | ||
| reporting_channel_size: 100, | ||
| reporting_timeout: Duration::from_millis(1), | ||
| engine_events: SendPolicy { | ||
| blocking_timeout: Some(Duration::from_millis(1)), | ||
| console_fallback: true, | ||
| }, | ||
| logging_events: SendPolicy { | ||
| blocking_timeout: None, | ||
| console_fallback: false, | ||
| }, | ||
| } | ||
| } | ||
| } |
utpilla
reviewed
Jan 21, 2026
…d/internal_logging_7
utpilla
reviewed
Jan 21, 2026
utpilla
approved these changes
Jan 21, 2026
Contributor
utpilla
left a comment
utpilla
left a comment
There was a problem hiding this comment.
Left some suggestions to cleanup code.
Merged
via the queue into
open-telemetry:main
with commit Jan 21, 2026
9ef8217
43 of 44 checks passed
cijothomas
added a commit
to cijothomas/otel-arrow
that referenced
this pull request
Jan 22, 2026
…y#1841) Part of open-telemetry#1771. Part of open-telemetry#1736. Implements 4 of the 5 ProviderMode values. Uses the ObservedStateStore's associated thread and channel to process console_async messages. Replaces most of open-telemetry#1771. Undoes portions of open-telemetry#1818: - ObservedEvent is an enum for Engine, Log events - Engine events return to `Option<String>` message, no structured message - Removes info_event! and error_event! structured message constructor macros - Moves LogRecord::Serialize support to where it's used Adds new LoggingProviders selector `admin` to configure how the admin threads use internal logging. The new setting defaults to ConsoleDirect, i.e., the admin components will use synchronous console logging. Configures the Tokio tracing subscriber globally, in engine threads, and in admin threads according to the ProviderMode. The asynchronous tracing subscriber (which sends to console_async; will send to ITS in the future) uses the `internal` provider mode itself as a fallback. However, it does this directly, choosing the Noop or ConsoleDirect modes, OpenTelemetry mode is not supported here. ~Resolves a TODO about inconsistency in the otel_xxx! macros. These now support full Tokio syntax following raw_error!~ EDIT: portions of this PR were moved into open-telemetry#1843. This PR removes the top-level `log` dependency. --------- Co-authored-by: Cijo Thomas <cithomas@microsoft.com> Co-authored-by: Lalit Kumar Bhasin <lalit_fin@yahoo.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #1771.
Part of #1736.
Implements 4 of the 5 ProviderMode values.
Uses the ObservedStateStore's associated thread and channel to process console_async messages.
Replaces most of #1771.
Undoes portions of #1818:
Option<String>message, no structured messageAdds new LoggingProviders selector
adminto configure how the admin threads use internal logging. The new setting defaults to ConsoleDirect, i.e., the admin components will use synchronous console logging.Configures the Tokio tracing subscriber globally, in engine threads, and in admin threads according to the ProviderMode.
The asynchronous tracing subscriber (which sends to console_async; will send to ITS in the future) uses the
internalprovider mode itself as a fallback. However, it does this directly, choosing the Noop or ConsoleDirect modes, OpenTelemetry mode is not supported here.Resolves a TODO about inconsistency in the otel_xxx! macros. These now support full Tokio syntax following raw_error!EDIT: portions of this PR were moved into #1843. This PR removes the top-level
logdependency.