-
Notifications
You must be signed in to change notification settings - Fork 440
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
[Exporter] Group spans by resource and instrumentation scope in OTLP export requests #2335
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2335 +/- ##
==========================================
+ Coverage 87.43% 87.45% +0.02%
==========================================
Files 199 199
Lines 5997 5997
==========================================
+ Hits 5243 5244 +1
+ Misses 754 753 -1 |
This reverts commit ed3e87e.
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. Nicely done. We should do similar optimization for logs too. Will raise a issue for that, so someone can pick it.
{ | ||
*scope_spans->add_spans() = std::move(input_span->span()); | ||
} | ||
} |
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 - perhaps we can remove OtlpRecordable::ProtoResource()
, OtlpRecordable::GetProtoInstrumentationScope()
and OtlpRecordable::GetInstrumentationLibrarySchemaURL()
methods now that they are not used to populate request object.
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, and very clean fix.
This PR changes the implementation of
OtlpRecordableUtils::PopulateRequest
to group spans by their resource and instrumentation scope.Instead of duplicating the resource and instrumentation scope for every span this now collects spans with the same resources and instrumentation scopes, reducing the size of the resulting
proto::collector::trace::v1::ExportTraceServiceRequest
.For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes