Switch v1 collector pipeline to v2 Writer #6491
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Yuri Shkuro <github@ysh.us>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6491 +/- ##
===========================================
+ Coverage 50.22% 96.25% +46.02%
===========================================
Files 188 372 +184
Lines 11403 21360 +9957
===========================================
+ Hits 5727 20559 +14832
+ Misses 5218 610 -4608
+ Partials 458 191 -267
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Signed-off-by: Yuri Shkuro <github@ysh.us>
| @@ -116,7 +117,7 @@ func TestNewCollector(t *testing.T) { | |||
| ServiceName: "collector", | |||
| Logger: logger, | |||
| MetricsFactory: baseMetrics, | |||
| SpanWriter: spanWriter, | |||
| TraceWriter: v1adapter.NewTraceWriter(spanWriter), | |||
There was a problem hiding this comment.
instead of the wrapping the existing spanWriter, is it possible for spanWriter to be natively of type tracestore.Writer?
There was a problem hiding this comment.
This will have a large knock-off effect on many tests, which are later introspecting the data in v1 model. I don't think it's worth it. In the follow-up PRs where we actually enable native v2 writer in the handlers there might be other tests for v2 code paths.
Signed-off-by: Yuri Shkuro <github@ysh.us>
| } | ||
|
|
||
| sp.queue.StartConsumers(sp.numWorkers, func(item queueItem) { | ||
| sp.processItemFromQueue(item) | ||
| }) | ||
|
|
||
| err = sp.otelExporter.Start(context.Background(), sp.telset.Host) |
There was a problem hiding this comment.
@yurishkuro what's the reason we're starting a new context here rather than propagating it from upstream?
There was a problem hiding this comment.
same reason, trying to minimize the changes
|
we have some missing code coverage - are these code paths not testable? |
mahadzaryab1
approved these changes
Jan 12, 2025
|
there are two forms of missing coverage, one is for errors coming from OTEL constructor, which is quite difficult to induce, and the other is from the writer errors - those will get added once we actually start using the v2 writer. |
yurishkuro
added a commit
that referenced
this pull request
Jan 13, 2025
## Which problem is this PR solving? - Context is lost in the process, see #6491 (comment) ## Description of the changes - Add Context argument to handler and span processor methods ## How was this change tested? - CI --------- Signed-off-by: Yuri Shkuro <github@ysh.us>
ekefan
pushed a commit
to ekefan/jaeger
that referenced
this pull request
Jan 14, 2025
## Which problem is this PR solving? - Part of jaegertracing#6487 - Part of jaegertracing#6474 ## Description of the changes - Swap v1 spanWriter for v2 traceWriter in collector pipeline - Currently the traceWriter is provided via v1 adapter, so it's always v1 writer underneath - And since only v1 spans entry point is currently implemented, there is no performance impact from additional data transformations - However, as soon as OTLP entry point is utilized (e.g. via OTLP receiver), the `ptrace.Traces` batch will be handled via exporterhelp queue as a single item (not broken into individual spans) and then passed directly to the writer as a batch. Since the writer is implemented via adapter the batch will be converted to spans and written one span at a time. There will be no additional data transformations on this path either. ## How was this change tested? - CI ## Outstanding - [x] Invoking proper preprocessing, like sanitizers and collector tags, on the OTLP path - [x] Adequate metrics parity, ideally same as v1 collector - [ ] Test coverage, including passing a v2-like (mock) writer that cannot be downgraded to v1 - Idea: parameterize some tests (ideally those that also validate pre-processing) to execute both v1 and v2 write paths ## Follow-up PRs * Enable v2 write path from OTLP and Zipkin receivers (they currently explicitly downgrade to v1). This will also allow adding better unit tests. --------- Signed-off-by: Yuri Shkuro <github@ysh.us> Signed-off-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
ekefan
pushed a commit
to ekefan/jaeger
that referenced
this pull request
Jan 14, 2025
## Which problem is this PR solving? - Context is lost in the process, see jaegertracing#6491 (comment) ## Description of the changes - Add Context argument to handler and span processor methods ## How was this change tested? - CI --------- Signed-off-by: Yuri Shkuro <github@ysh.us>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which problem is this PR solving?
Description of the changes
ptrace.Tracesbatch will be handled via exporterhelp queue as a single item (not broken into individual spans) and then passed directly to the writer as a batch. Since the writer is implemented via adapter the batch will be converted to spans and written one span at a time. There will be no additional data transformations on this path either.How was this change tested?
Outstanding
Follow-up PRs