Redesign Trace Timeline #2736
Redesign Trace Timeline #2736
Conversation
Current state
|
Current State
In #2525 , I said that I wanted to set the span details on the right side of the page.
|
Current State
|
|
I like it, I'd love to see what it looks like with some of the more extreme example data. Like the depth > 10 data or 1000+ spans |
|
Thanks for starting this effort. it is daring work! I have an image with feedback. I think this is also a good time to add "site tags" at a stable place in the view as discussed before, though not necessarily in this change.
cc @openzipkin/ui |
tacigar
force-pushed
the
tacigar/redesign-timeline
branch
from
5fd48d8
to
09c3aa6
Compare
|
somethings noticed: ex show "80f198ee56343ba864fe8b2a57d3eff7-e457b5a2e4d86bd1" if rooted to the ID e457b5a2e4d86bd1 in the trace 80f198ee56343ba864fe8b2a57d3eff7 nice plus would be to instead of showing http://localhost:9411/zipkin/traces/80f198ee56343ba864fe8b2a57d3eff7 http://localhost:9411/zipkin/traces/80f198ee56343ba864fe8b2a57d3eff7-e457b5a2e4d86bd1 I'm using hyphen format as that is the same as b3 single format https://github.com/openzipkin/b3-propagation#single-header |
|
yes no duration is same as zero duration. valid duration starts at 1us. in
other words round up. this should mean truthy comparison is ok.
…On Fri, Oct 4, 2019, 4:25 PM tacigar ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In zipkin-lens/src/components/TracePage/TraceSummary.jsx
<#2736 (comment)>:
> + if (allHiddenSpanIds[span.spanId] && span.childIds) {
+ span.childIds.forEach((childId) => {
+ allHiddenSpanIds[childId] = true;
+ });
+ }
+ });
+ return rerootedTree.filter(span => !allHiddenSpanIds[span.spanId]);
+ }, [rerootedTree, childrenHiddenSpanIds]);
+
+ // Find the minumum and maximum timestamps in the shown spans.
+ const startTs = useMemo(() => minBy(rerootedTree, 'timestamp').timestamp, [rerootedTree]);
+ const endTs = useMemo(() => {
+ let max = 0;
+ rerootedTree.forEach((span) => {
+ let ts;
+ if (!span.duration) {
At least I found spans that don't have a duration in teatdata.
https://github.com/openzipkin/zipkin/blob/2c353e17c0a2fa8440fe01ba924aff36dd416aa3/zipkin-lens/testdata/netflix.json#L2-L41
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2736?email_source=notifications&email_token=AAAPVV6LGZA6R3YWRRVHQJLQM34YTA5CNFSM4IJHKXW2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCG4NXOY#discussion_r331390720>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAPVVZK4CYTEEUX2OXZNM3QM34YTANCNFSM4IJHKXWQ>
.
|
|
on duration being only valid starting at 1:
https://github.com/openzipkin/zipkin-api/blob/master/zipkin2-api.yaml#L464
you can make a comment like this
|
|
If the API leaves out duration in some cases, it would be nice to set a duration of |
|
I dont quite understand what problem we are solving here. duration query is
expensive so we certainly should not add a constraint like that by default
especially since the query is trace scoped not span scoped. a 1us query
will match even a trace that has spans minus a duration.
also note that Netflix span here was a converted one from a different
system. it is not representative of a proper span, but a flushed span can
also be missing a duration.
…On Fri, Oct 4, 2019, 4:58 PM Anuraag Agrawal ***@***.***> wrote:
If the API leaves out duration in some cases, it would be nice to set a
duration of 1 when fetching and duration is missing since that is the
restraint set by the model, and then we don't need to have as many checks
at business logic call sites.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2736?email_source=notifications&email_token=AAAPVV2IUJUJP7R5CP5E2LDQM4AUFA5CNFSM4IJHKXW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEALAKJY#issuecomment-538314023>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAPVV663IKYDARR6NBYCU3QM4AUFANCNFSM4IJHKXWQ>
.
|
|
I didn't mean changing the query - just if the JavaScript encounters a span
with no duration it can fill in 1. The problem that is solved is that
instead of every call site having to check for empty duration, only one
does. It's not expensive.
…On Fri, Oct 4, 2019, 19:03 Adrian Cole ***@***.***> wrote:
I dont quite understand what problem we are solving here. duration query is
expensive so we certainly should not add a constraint like that by default
especially since the query is trace scoped not span scoped. a 1us query
will match even a trace that has spans minus a duration.
also note that Netflix span here was a converted one from a different
system. it is not representative of a proper span, but a flushed span can
also be missing a duration.
On Fri, Oct 4, 2019, 4:58 PM Anuraag Agrawal ***@***.***>
wrote:
> If the API leaves out duration in some cases, it would be nice to set a
> duration of 1 when fetching and duration is missing since that is the
> restraint set by the model, and then we don't need to have as many checks
> at business logic call sites.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <
#2736?email_source=notifications&email_token=AAAPVV2IUJUJP7R5CP5E2LDQM4AUFA5CNFSM4IJHKXW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEALAKJY#issuecomment-538314023
>,
> or mute the thread
> <
https://github.com/notifications/unsubscribe-auth/AAAPVV663IKYDARR6NBYCU3QM4AUFANCNFSM4IJHKXWQ
>
> .
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2736?email_source=notifications&email_token=AABQNSEXM2EHHAVV3Q2NU3LQM4IFRA5CNFSM4IJHKXW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEALFRBQ#issuecomment-538335366>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABQNSEWI6ECIUUOH7UGAYLQM4IFRANCNFSM4IJHKXWQ>
.
|
|
no duration isnt same as 1 duration. I prefer we dont fix like that. there
is a span cleaner javascript
On Fri, Oct 4, 2019, 7:39 PM Anuraag Agrawal <notifications@github.com>
wrote:
… I didn't mean changing the query - just if the JavaScript encounters a span
with no duration it can fill in 1. The problem that is solved is that
instead of every call site having to check for empty duration, only one
does. It's not expensive.
On Fri, Oct 4, 2019, 19:03 Adrian Cole ***@***.***> wrote:
> I dont quite understand what problem we are solving here. duration query
is
> expensive so we certainly should not add a constraint like that by
default
> especially since the query is trace scoped not span scoped. a 1us query
> will match even a trace that has spans minus a duration.
>
>
> also note that Netflix span here was a converted one from a different
> system. it is not representative of a proper span, but a flushed span can
> also be missing a duration.
>
> On Fri, Oct 4, 2019, 4:58 PM Anuraag Agrawal ***@***.***>
> wrote:
>
> > If the API leaves out duration in some cases, it would be nice to set a
> > duration of 1 when fetching and duration is missing since that is the
> > restraint set by the model, and then we don't need to have as many
checks
> > at business logic call sites.
> >
> > —
> > You are receiving this because you were mentioned.
> > Reply to this email directly, view it on GitHub
> > <
>
#2736?email_source=notifications&email_token=AAAPVV2IUJUJP7R5CP5E2LDQM4AUFA5CNFSM4IJHKXW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEALAKJY#issuecomment-538314023
> >,
> > or mute the thread
> > <
>
https://github.com/notifications/unsubscribe-auth/AAAPVV663IKYDARR6NBYCU3QM4AUFANCNFSM4IJHKXWQ
> >
> > .
> >
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <
#2736?email_source=notifications&email_token=AABQNSEXM2EHHAVV3Q2NU3LQM4IFRA5CNFSM4IJHKXW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEALFRBQ#issuecomment-538335366
>,
> or mute the thread
> <
https://github.com/notifications/unsubscribe-auth/AABQNSEWI6ECIUUOH7UGAYLQM4IFRANCNFSM4IJHKXWQ
>
> .
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2736?email_source=notifications&email_token=AAAPVVYDSGWERDVSYAOB5DTQM4TQLA5CNFSM4IJHKXW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEALMDGY#issuecomment-538362267>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAPVV47VR72SLPZM6FPCMDQM4TQLANCNFSM4IJHKXWQ>
.
|
|
Since the yaml you linked indicated minimum duration is 1, I figured you were trying to say no duration is the same as 1 but apparently not. Anyways going to go ahead and pull out of this PR since this isn't helping. |
|
@anuraaga About the duration 1 issue. trying to solve the programming cleanliness indeed does help. Duration 1 as a solution raised strong antibodies, but I don't think that was your original intent. I wanted to make sure that didn't happen as then we would have to immediately undo it. I'm sorry as I was literally presenting at the time, it was hard for me to give complete context. I'm most concerned about adding a 1 duration as that will make it hard to see errors or incomplete spans. Also, I'm worried about alignments and things, or people wondering why it took 1us. We have an existing issue somewhere to alert people about broken or incomplete traces, and don't want to make that harder to do. That's why I had strong pushback about coercing absent duration to 1.
So, to put it all together :) I didn't say explicitly, wasn't online really or anything and it was hard for me to see the contexts of the different angles.. but here's the advice. It is normal to have span cleaning, knowing duration is 0 is same as absent, then change span-cleaner to backfill zero duration. I think this will solve the programming problem. Adding (getting total duration of a row, which I think this comment was about) is ok. Main thing if we do this is make sure code that does percentage skips span with zero duration: we don't yet know their duration. Be careful to check, that's all. I don't even remember if we do any percentage anymore in the UI, so this point might be a relic of the old one. |
|
nit: |
|
+1 to |
|
Changes look really nice! All of the images showing the progression and feedback in the PR is really helpful so thanks to everyone for taking time on those! |
|
@tacigar no pressure, but what's the status on this? |
|
@tacigar if you are swamped, I can try rebasing and addressing remaining review comments. Generally, it is a good idea to keep large change contained and I think this is nearly there. Lemme know. |
|
Sorry, I was busy at another work. |
Thank you for your feedback! |
|
PTAL 🙇 |
|
I have high confidence. just local testing now. |
|
This is really so good. great work @tacigar! |
resolve #2672
resolve #2525
resolve #2195