Merge span normalizer into grouper #384
Conversation
This comment has been minimized.
This comment has been minimized.
Codecov Report
@@ Coverage Diff @@
## main #384 +/- ##
============================================
+ Coverage 79.51% 79.61% +0.10%
+ Complexity 1410 1401 -9
============================================
Files 126 125 -1
Lines 5545 5525 -20
Branches 520 519 -1
============================================
- Hits 4409 4399 -10
+ Misses 900 891 -9
+ Partials 236 235 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| output.topic = "structured-traces-from-raw-spans" | ||
| bypass.output.topic = "structured-traces-from-raw-spans" |
There was a problem hiding this comment.
Do we still need this bypass.output.topic. Once we merge the topology, we can probably reuse output.topic
|
Is there a reason for removing span normalizer completely? This is a breaking change and will cause failure in our on-prem platform? |
|
|
||
| KStream<TraceIdentity, RawSpan> inputStream = | ||
| (KStream<TraceIdentity, RawSpan>) inputStreams.get(inputTopic); | ||
| KStream<byte[], JaegerSpanInternalModel.Span> inputStream = |
There was a problem hiding this comment.
Needs to be an incremental change. We need to read from both jaeger-spans and raw-spans-from-jaeger-spans topics in this PR.
Only in follow up PR some time down the line, we can just read from jaeger-spans
| @@ -75,7 +89,34 @@ public StreamsBuilder buildTopology( | |||
| streamsBuilder.addStateStore(spanStoreBuilder); | |||
| streamsBuilder.addStateStore(traceStateStoreBuilder); | |||
|
|
|||
| StreamPartitioner<TraceIdentity, StructuredTrace> groupPartitioner = | |||
| KStream<byte[], PreProcessedSpan> preProcessedStream = | |||
| inputStream.transform(() -> new JaegerSpanPreProcessor(getGrpcChannelRegistry())); | |||
There was a problem hiding this comment.
How are we ensuring that this jaeger spans topic is consumed exactly from the offset where the span-normalizer got decommissioned?
There was a problem hiding this comment.
Good point. Being different applications, it will be difficult to get them in sync.
There was a problem hiding this comment.
My suggestion would be the following:
We shouldn't even be reading from the existing jaeger-spans topic. This topology should read from a different topic(say jaeger-spans-v2). On the producer side(i.e. hypertrace-collector), we should have a way to control whether to produce to the existing jaeger-spans topic or to the new jaegar-spans-v2. Once we make the flip to the v2 topic we can decommission span-normalizer.
There was a problem hiding this comment.
Probably need to keep both applications for now. And may need to move raw-span-grouper logic to span-normalizer instead of other way around.
This change merges span-normalizer into raw-spans-grouper to reduce 1 redundant output topic (raw-spans-from-jaeger-spans)