[v2][adjuster] Implement otlp to model translator with post processing #6394
Conversation
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
mahadzaryab1
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6394 +/- ##
=======================================
Coverage 96.23% 96.24%
=======================================
Files 364 365 +1
Lines 20868 20907 +39
=======================================
+ Hits 20082 20121 +39
Misses 601 601
Partials 185 185
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
internal/jptrace/translator.go
Outdated
|
|
||
| func ProtoFromTraces(traces ptrace.Traces) []*model.Batch { | ||
| batches := otlp2jaeger.ProtoFromTraces(traces) | ||
| for i, batch := range batches { |
There was a problem hiding this comment.
Seems unsafe, is it guaranteed that batches and resources have identical cardinality?
There was a problem hiding this comment.
@yurishkuro I was following the logic in the upstream translator: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/pkg/translator/jaeger/traces_to_jaegerproto.go#L25. Let me know if I'm misunderstanding.
There was a problem hiding this comment.
but how do we know upstream logic won't change? A robust way would be to build a map of spanId->*model.Span first and then loop through ptrace spans, find corresponding model.Span and transfer attributes into warnings.
btw, we should probably delete those attributes from model.Span, since ptrace would just copy them.
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
internal/jptrace/translator.go
Outdated
| spans := scopes.At(j).Spans() | ||
| for k := 0; k < spans.Len(); k++ { | ||
| span := spans.At(k) | ||
| spanMap[span.SpanID()] = span |
There was a problem hiding this comment.
I think it would be better the other way. You can't guarantee no dups in ptrace. If you keep a map for model.Span then you guarantee that all warnings from ptrace will be captured
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
jaegertracing#6394) ## Which problem is this PR solving? - Towards jaegertracing#6344 ## Description of the changes - Implemented a translator in `jptrace` with a function `ProtoFromTraces` that is a wrapper of the upstream `ProtoFromTraces` in opentelemetry-collector-contrib jaeger translator. This function appends the warnings in `jaeger.internal.warnings` to the corresponding warnings field in the proto model. ## How was this change tested? - Added unit tests ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `npm run lint` and `npm run test` --------- Signed-off-by: Mahad Zaryab <[email protected]>
Which problem is this PR solving?
Description of the changes
jptracewith a functionProtoFromTracesthat is a wrapper of the upstreamProtoFromTracesin opentelemetry-collector-contrib jaeger translator. This function appends the warnings injaeger.internal.warningsto the corresponding warnings field in the proto model.How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test