[v2][adjuster] Implement span ID uniquifier adjuster to operate on otlp data model #6367
Conversation
Signed-off-by: Mahad Zaryab <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6367 +/- ##
==========================================
+ Coverage 96.16% 96.20% +0.04%
==========================================
Files 360 361 +1
Lines 20513 20600 +87
==========================================
+ Hits 19727 19819 +92
+ Misses 601 597 -4
+ Partials 185 184 -1
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: 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]>
| serverSpan := spans.At(1) | ||
| assert.EqualValues(t, []byte{0, 0, 0, 0, 0, 0, 0, 2}, serverSpan.SpanID(), "server span ID should be reassigned") | ||
| assert.EqualValues(t, []byte{0, 0, 0, 0, 0, 0, 0, 3}, serverSpan.ParentSpanID(), "next server span should be this server span's parent") | ||
|
|
||
| anotherServerSpan := spans.At(2) | ||
| assert.EqualValues(t, []byte{0, 0, 0, 0, 0, 0, 0, 3}, anotherServerSpan.SpanID(), "server span ID should be reassigned") | ||
| assert.EqualValues(t, clientSpanID, anotherServerSpan.ParentSpanID(), "client span should be server span's parent") |
There was a problem hiding this comment.
@yurishkuro does this expectation make sense to you? it takes the last repeated span and sets that to be the parent and then adjusts the repeated ones before to have that be the parent. So in this case, the hierarchy becomes clientSpan -> anotherServerSpan -> serverSpan.
There was a problem hiding this comment.
first, when you have one client and two server spans sharing the same ID, it's doesn't really make sense and the behavior of adjuster is not well defined, because the condition already violates "shared span id" notion from Zipkin. I think it's technically possible to have if a retry from client was done without creating a new client span. I think a reasonable thing to do in such case is to assign unique IDs to both server spans, such that we get a tree (but not a chain that you have):
clientSpan -> serverSpan
clientSpan -> anotherServerSpan
``
In addition, if there is a 4th span whose `parentId == clientSpanId`, then that 4th span's parent ID must be overwritten to the newly generated span ID. Here we have an ambiguity - it could be either `serverSpan.spanId` or `anotherServerSpan.spanId`, there's no way for us to know (and we should arbitrarily pick first or last reassigned `server` span).
There was a problem hiding this comment.
@yurishkuro Ah okay. I'm happy to remove this test if the behaviour is undefined then because it didn't exist in the old implementation either.
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
force-pushed
the
zipkin-span-id-uniqify
branch
from
1630e01 to
6042204
Compare
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
|
|
||
| serverSpan := spans.At(1) | ||
| assert.EqualValues(t, []byte{0, 0, 0, 0, 0, 0, 0, 2}, serverSpan.SpanID(), "server span ID should be reassigned") | ||
| assert.EqualValues(t, []byte{0, 0, 0, 0, 0, 0, 0, 3}, serverSpan.ParentSpanID(), "next server span should be this server span's parent") |
There was a problem hiding this comment.
I would expect serverSpan.parentSpanId == clientSpanID. If it's id=3, there was no such span in the trace before, so it's a broken link.
There was a problem hiding this comment.
@yurishkuro That's what i was expecting as well but this is a direct translation of what was there before. If you take a look at the old implementation, oldToNewSpanIDs gets overwritten with the latest trace that it encounters which is what causes this. Do we want to change the behaviour here?
There was a problem hiding this comment.
This was due to the undefined behaviour resulting from a having two server spans sharing the same clientID. This is now fixed.
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
| serverSpan := spans.AppendEmpty() | ||
| serverSpan.SetTraceID(traceID) | ||
| serverSpan.SetSpanID(clientSpanID) // shared span ID | ||
| serverSpan.SetKind(ptrace.SpanKindServer) |
There was a problem hiding this comment.
so you dropped the edge case of two server spans sharing client span id? Maybe it's for the better - we always wanted to support partial spans where we could have 2+ span "chunks" sharing the ID which should be merged into a single span. To support partial spans and this use case of dual server spans sharing client span ID we'd need to be very careful about examining the Resource/Scope of the spans to ensure they are identical (or one is empty).
There was a problem hiding this comment.
@yurishkuro Yeah I dropped it given that the behaviour is undefined and in favour of keeping the implementation the same as the existing adjuster. Happy to revisit this again in the future if we want to support partial spans as you suggested.
Signed-off-by: Mahad Zaryab <[email protected]>
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test