-
Notifications
You must be signed in to change notification settings - Fork 69
Console exporter for OTAP/OTLP logs #1849
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 #1849 +/- ##
==========================================
- Coverage 84.72% 84.69% -0.03%
==========================================
Files 503 504 +1
Lines 149956 150323 +367
==========================================
+ Hits 127048 127322 +274
- Misses 22374 22467 +93
Partials 534 534
🚀 New features to boost your workflow:
|
| // Copyright The OpenTelemetry Authors | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| //! Console exporter that prints OTLP data with hierarchical formatting. |
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.
do we want to offer stability guarantees about output format or we want to explicitly mention that output format is not stable and not recommended to be relied for automation/parsing.?
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.
Good question. We've seen users relying on SDK's ConsoleExporter in production with the expectation that the output format won't change.
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.
Noted as unstable for now! We can open an explicit stabilization issue.
| let event_name = log_record | ||
| .event_name() | ||
| .map(|s| String::from_utf8_lossy(s).into_owned()) | ||
| .unwrap_or_else(|| "event".to_string()); |
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.
why "event" instead of leaving it empty?
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.
That was a mistake, fixed.
| OtapPayload::OtlpBytes(bytes) => { | ||
| self.formatter.format_logs_bytes(bytes); | ||
| } | ||
| OtapPayload::OtapArrowRecords(records) => match OtapLogsView::try_from(records) { | ||
| Ok(logs_view) => { | ||
| self.formatter.format_logs_arrow(&logs_view); | ||
| } |
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'd be nice to implement impl<'a> TryFrom<&'a OtlpProtoBytes> for OtapLogsView<'a> as well similar to OtapArrowRecords. That would simplify the code here by having both paths go through the same set of steps.
OtlpBytes/OtapArrowRecords -> OtapLogsView -> Write to 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.
@utpilla - Wondering if this would need decoding the protobuf into rows and then building Arrow arrays (and the OTAP layout). How about keeping RawLogsData for OTLP bytes and OtapLogsView for Arrow; a generic format_logs<L: LogsDataView> to keep call sites tidy?
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.
This should not require decoding! Trying...
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.
Oh yes, but looking into the code, there may be lifetime issue.
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.
I cleaned this up somewhat, but yes, I'm not sure how to handle this kind of try-various-impls pattern here. I did add a try_from so that at least the two are symmetric:
fn export_logs(&self, payload: &OtapPayload) {
match payload {
OtapPayload::OtlpBytes(bytes) => match RawLogsData::try_from(bytes) {
Ok(logs_view) => {
self.formatter.print_logs_data(&logs_view);
}
Err(e) => {
otel_error!("Failed to create OTLP logs view", error = ?e);
}
},
OtapPayload::OtapArrowRecords(records) => match OtapLogsView::try_from(records) {
Ok(logs_view) => {
self.formatter.print_logs_data(&logs_view);
}
Err(e) => {
otel_error!("Failed to create OTAP logs view", error = ?e);
}
},
}
}
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.
This looks good actually. The trait-based approach is the right pattern here.
|
|
||
| impl ConsoleExporter { | ||
| fn export(&self, data: &OtapPdata) { | ||
| let (_, payload) = data.clone().into_parts(); |
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.
We could avoid the cloning of payload here for each export. We are only using references here in the export_* methods. It could be worth adding another method to OtapPData which returns the reference to payload without consuming the payload:
/// Returns a reference to the payload
#[must_use]
pub const fn payload_ref(&self) -> &OtapPayload {
&self.payload
}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! 3985183
| .map(nanos_to_time) | ||
| .unwrap_or(SystemTime::UNIX_EPOCH); | ||
|
|
||
| let prefix = format!("{} ", self.tree.vertical); |
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.
This would allocate a new string for each scope. Since tree.vertical is a &static str, we could pre-compute the prefix as a 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.
Fixed!
| // Format each scope | ||
| let scopes: Vec<_> = resource_logs.scopes().collect(); | ||
| let scope_count = scopes.len(); | ||
| for (i, scope_logs) in scopes.into_iter().enumerate() { | ||
| let is_last_scope = i == scope_count - 1; | ||
| self.format_scope_logs_to(&scope_logs, is_last_scope, output); | ||
| } |
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 looks like we're allocating a vec here just to identify the last element. Could we use peekable() instead to avoid the allocation?
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.
| // Format each log record | ||
| let records: Vec<_> = scope_logs.log_records().collect(); | ||
| let record_count = records.len(); | ||
| for (i, log_record) in records.into_iter().enumerate() { | ||
| let is_last_record = i == record_count - 1; | ||
| self.format_log_record_to(&log_record, is_last_scope, is_last_record, output); | ||
| } |
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.
Same as #1849 (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.
Fixed! Thanks. 978d44b
| let version = scope | ||
| .as_ref() | ||
| .and_then(|s| s.version()) | ||
| .map(|v| String::from_utf8_lossy(v).into_owned()); |
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.
We could rid of the into_owned and avoid the string allocation here by using a Cow<str> or raw bytes directly.
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 (raw bytes works).
| pub fn format_logs_bytes(&self, bytes: &OtlpProtoBytes) { | ||
| let mut output = Vec::new(); | ||
| self.format_logs_bytes_to(bytes, &mut output); | ||
| let _ = std::io::stdout().write_all(&output); |
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.
Use the async version?
let _ = tokio::io::stdout().write_all(&output).await;
| pub fn format_logs_arrow(&self, logs_view: &OtapLogsView<'_>) { | ||
| let mut output = Vec::new(); | ||
| self.format_logs_data_to(logs_view, &mut output); | ||
| let _ = std::io::stdout().write_all(&output); |
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.
Do we want to log errors here if write_all fails?
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.
Now reads:
if let Err(err) = tokio::io::stdout().write_all(&output).await {
otel_error!("could not write to console", error = ?err);
}
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.
Most of my comments are perf related which may not be as much of a concern for a ConsoleExporter.
There is one comment about using the std I/O blocking write_all vs tokio's async std I/O which I think we should address.
|
I appreciate all of this feedback! I see this component function as not a high priority, however all the points made about performance are likely to impact other export paths so I wouldn't dismiss any it. |
| /// Write severity number with appropriate color and padding. | ||
| /// Severity numbers follow OTLP conventions (1-24, where INFO=9). | ||
| #[inline] | ||
| pub fn write_severity(&self, w: &mut BufWriter<'_>, severity: Option<i32>) { |
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.
should we extend this method to also write severity-text along with the severity number?
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.
ah, yes! I had not considered severity text since I dropped it from the LogRecord encoding as being redundant for the 5 Tokio levels.
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.
Added, tested.
| where | ||
| F: FnOnce(&mut BufWriter<'_>), | ||
| { | ||
| let mut buf = [0u8; LOG_BUFFER_SIZE]; |
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.
More for later PR, should be good to add TODO for now - this is silently truncating if the writable size if over 4KB. Which means - A log record with a big body or lots of attributes will get cut off without warning. We can detect overflow, and append truncation marker, so user doesn't get misled with truncated content. Or else not to truncate by using growable buffer.
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.
I have this and some related topics recorded in #1746 🚀
lalitb
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, with non-blocking comments.
…o jmacd/console_exp_9
Co-authored-by: Utkarsh Umesan Pillai <66651184+utpilla@users.noreply.github.com>
2fae2f7
Part of open-telemetry#1771. Part of open-telemetry#1736. Uses the new internal logging support to format OTLP logs data. This prints RESOURCE and SCOPE lines with ASCII or Unicode pipe structures to identify the OTLP hierarchy: ``` 2026-01-21T03:12:22.165Z RESOURCE v1.Resource: [fake_data_generator=v1] 2026-01-21T03:12:22.165Z │ SCOPE v1.InstrumentationScope: 2026-01-21T03:12:22.165Z │ ├─ INFO session.start: [session.id=00112233-4455-6677-8899-aabbccddeeff, session.previous_id=00112233-4455-6677-8899-aabbccddeeff] 2026-01-21T03:12:22.165Z │ ├─ INFO session.end: [session.id=00112233-4455-6677-8899-aabbccddeeff] 2026-01-21T03:12:22.165Z │ ├─ INFO device.app.lifecycle: [ios.app.state=active, android.app.state=created] 2026-01-21T03:12:22.165Z │ ├─ INFO rpc.message: [rpc.message.type=SENT, rpc.message.id=42, rpc.message.compressed_size=42, rpc.message.uncompressed_size=42] ``` --------- Co-authored-by: Utkarsh Umesan Pillai <66651184+utpilla@users.noreply.github.com>
Part of #1771.
Part of #1736.
Uses the new internal logging support to format OTLP logs data. This prints RESOURCE and SCOPE lines with ASCII or Unicode pipe structures to identify the OTLP hierarchy: