-
Notifications
You must be signed in to change notification settings - Fork 484
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
Log SDK optimization - Improve perf 15%-30% #2043
Log SDK optimization - Improve perf 15%-30% #2043
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2043 +/- ##
=======================================
- Coverage 77.1% 77.1% -0.1%
=======================================
Files 123 123
Lines 21104 21103 -1
=======================================
- Hits 16275 16274 -1
Misses 4829 4829 ☔ View full report in Codecov by Sentry. |
let provider = self.provider(); | ||
let processors = provider.log_processors(); | ||
let trace_context = Context::map_current(|cx| { | ||
cx.has_active_span() | ||
.then(|| TraceContext::from(cx.span().span_context())) | ||
}); | ||
let mut log_record = record; | ||
|
||
//let mut log_record = record; |
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: is this comment required?
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, will remove it in next PR.
fn emit(&self, data: &mut LogData); | ||
/// - `record`: A mutable reference to `LogData` representing the log record. | ||
/// - `instrumentation`: The instrumentation library associated with the log record. | ||
fn emit(&self, data: &mut LogRecord, instrumentation: &InstrumentationLibrary); |
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: for a future PR, we can provide empty implementation for this as well.
@@ -9,6 +9,7 @@ | |||
~38 M /sec |
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.
Please update this, if there are any changes.
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.
Yes this should be ~44K, will update the benchmark and stress stats with preallocate attribute PR.
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.
Nice improvements! Please add a changelog and we can merge.
Merging to keep progressing, changelog can added later, before release. |
Changes
Optimize Log SDK by avoiding unnecessary copy operation and redirections
LogData
structure is removed.Logger::emit()
method is modified for unnecessary copy/assignment operation.LogProcessor::emit
method is modified to directly take reference ofLogRecord
andInstrumentationLibrary
instead ofLogData
. This simplifies the API, and more importantly ones less level of redirection while accessing these components.Existing:
Updated:
Benchmarks and stress: (courtesy chatgpt to generate table from the results):
ot_layer_enabled
Logging_Comparable_To_Appender
log_noop_processor
log_cloning_processor
log_clone_and_send_to_channel_processor
LogExporterWithFuture
LogExporterWithoutFuture
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes