-
Notifications
You must be signed in to change notification settings - Fork 504
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
Simplify LogExporter::Export interface #2041
Simplify LogExporter::Export interface #2041
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2041 +/- ##
=======================================
+ Coverage 77.0% 77.1% +0.1%
=======================================
Files 123 123
Lines 21111 21104 -7
=======================================
+ Hits 16260 16276 +16
+ Misses 4851 4828 -23 ☔ View full report in Codecov by Sentry. |
let export_batch = batch | ||
.iter() | ||
.map(|log_data| (&log_data.record, &log_data.instrumentation)) | ||
.collect(); |
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.
BatchProcessor is now doing an extra cloning - 1st one occurs when it receives the &mut LogData
, so it can save it to channel. And its drained to a Vec! later, which was passed as-is to Exporter previously. Now another Vec! needs to be created to pass on to exporter.
Something we can revisit and address in the future.
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 operation is only creating references and collecting them, no cloning of data is happening.
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.
Isn't the collect making a new Vec! ?
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, I was talking specifically about cloning. Need to revisit vector later.
Co-authored-by: Cijo Thomas <cijo.thomas@gmail.com>
Co-authored-by: Cijo Thomas <cijo.thomas@gmail.com>
Changes
Before:
After:
This simplifies the processing for exporters, as they don't need to check whether the object is borrowed, or owned. Exporter needs to ensure to create the copy of LogRecord and/or InstrumentationLib whenever needed.
LogData
structure is not part of the export interface. So it has been moved fromopentelemetry_sdk::export::logs
toopentelemetry_sdk::logs::
namespace.Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes