zipkin
zipkin copied to clipboard
openzipkin
Fix a bug that graph is not rendered when timestamp is not set in span
I would recommend seeing if this can be merged because we should release zipkin soon due to code drift between armeria here and what's used in zipkin-gcp (possibly also zipkin-aws). Maybe update to latest armeria after deciding what to do here then cut a minor.
I agree with this Tommy's opinion.(https://github.com/openzipkin/zipkin/issues/2906#issuecomment-556940485) What do you think @adriancole ?
sorry about the late comment. @shakuzen's note was asking about if data would be valid if there's no timestamp based on the notes in our docs about it. It is still invalid, but the trace should render.
we've had users think that because our data format permits certain fields like localEndpoint and timestamp to be absent, that means the data is valid. It isn't valid, but there's a different question about whether the UI should crash (shouldn't) or what the UI should do about it (skip or backfill based on some heuristic)
The reason it can be confusing is that spans can be reported twice. ex span.flush() followed by span.finish(). There are 2 json in this case and span-cleaner in lens will merge those as one. This means that if one or the other has a timestamp, we are fine.
Spans that never started are a bug, in other words. Spans reported twice are not necessarily a bug. In any case a missing timestamp shouldn't crash the UI for the trace.
Given this, should we merge the change?
cc @jorgheymans as being the issue-master this topic is an important and frequently misunderstood one. I'm even thinking about finding a place in our openapi docs to double or triple underscore that nullability of a field does not imply it is good data to leave it out. Somehow we need to clearly say that "patching" a span can happen and in that case, consider the merged data when thinking of how to interpret fields.
Also sorry about being late, i'ld say merge this in pending this comment https://github.com/openzipkin/zipkin/pull/2926#discussion_r348915149 .
About making this clear in the openapi docs, I don't think this will buy us much. Having a trace diagnostic tool somewhere that is able to tell if a trace is more or less as we expect it or not, what is wrong, what we can compensate in the UI etc would be more helpful. It's along these lines https://github.com/openzipkin/zipkin/issues/1484 , but i'm more thinking a separate endpoint where you would upload a trace or give it the id of a trace that isn't showing and it spits out the diagnostics.
