-
Notifications
You must be signed in to change notification settings - Fork 405
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
Moving LogRecord implementation to SDK #1702
Conversation
opentelemetry-sdk/src/logs/record.rs
Outdated
|
||
/// Implementation for LogRecordBuilder | ||
#[derive(Debug, Default)] | ||
pub struct SdkLogRecordBuilder { |
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 really need a builder for LogRecord
? Looking at the trait all it provides are method to change fields. So LogRecord
by itself looks like a builder to me.
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, I think I over-engineered a bit. Have removed the builder trait now :)
Can you post benchmarks/stress tests with the changes, to confirm that we are not regressing either. (Just curiosity! I agree that this PR is the right direction) |
Have added the stress and benchmark results in the description. Surprisingly I see the improvement, but don't want to read much out of that as of now :) |
/// Span Id | ||
pub span_id: SpanId, | ||
/// Trace flags | ||
pub trace_flags: Option<TraceFlags>, |
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.
wondering why is this optional?
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, this was existing code in API. I can have a look into this separately.
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.
Waiting to add changelog, and remove the API to set span/trace context before approving.
Co-authored-by: Cijo Thomas <cithomas@microsoft.com>
Co-authored-by: Cijo Thomas <cithomas@microsoft.com>
changelog is added now. Thanks for the review. |
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.
Thanks! Great to see API getting leaner and delegating things to the SDK!
@lalitb also - can you update PR desc. to reflect the final change in the PR? (Set vs Add and anything else) Thank you! |
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.
Looks good. Thanks for the change
Fixes #1189
Changes
Opening as draft to get the initial feedback. The changes are:
LogRecord
traits in the API.LogRecord
with implementation in the SDK.Earlier:
With PR:
Note - with the new design, the log record creation through chaining required cloning eventually while emitting, so haven't implemented the chaining. The tracing appender also doesn't require chaining as all these log data are populated separately.
This allows us to change the internal SDK implementation for LogRecord without breaking the Bridge API. While there are no current perf benefits with the approach, it enables us to add more optimizations in the SDK to optionally allow the serialization of the log records in the export format without any intermediate allocations within the SDK. I plan to add those optimizations in subsequent PRs once this is finalized.
Benchmark
Stress Test (20 threads)
Main: ~35 million iterations/sec
PR: ~39 million iterations/sec
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes