Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor UI to internally use the v2 model #2217

Closed
codefromthecrypt opened this issue Oct 23, 2018 · 5 comments · Fixed by #2304
Closed

Refactor UI to internally use the v2 model #2217

codefromthecrypt opened this issue Oct 23, 2018 · 5 comments · Fixed by #2304
Labels
enhancement ui Zipkin UI

Comments

@codefromthecrypt
Copy link
Member

Right now, we pull data in v2 format #1802, but we still operate on it in v1.
@codefromthecrypt codefromthecrypt mentioned this issue Oct 23, 2018
Closed
@GuangmingLuo
Copy link

Hi,
I recently find that I can't search trace by id in this UI [Go to trace] when I use 128 bit traceId for both version 2.10.4 and 2.11, even though it does exist in the DB.
Does this relate to this issue, or maybe raise a new issue ?
BR

@codefromthecrypt
Copy link
Member Author

I will try start this by changing the UI logic step-by-step to eventually use v2 data:

  1. break shared v1 spans into single host spans, but still v1 format
    Ex. {"cs" "sr" "sa" "ca" "ss" "cr"} -> {"cs" "cr", "sa"}, {"sr" "ss", "ca"}
  2. add span.kind, remoteEndpoint annotation to the UI's model temporarily and use it
    {"cs" "cr", "sa"}, {"sr" "ss", "ca"} -> {kind = CLIENT, "cs" "cr", remoteEndpoint}, {kind = SERVER, "sr" "ss", remoteEndpoint}
  3. add localEndpoint annotation to the UI's model temporarily and use it
    {kind = CLIENT, "cs" "cr", remoteEndpoint}, {kind = SERVER, "sr" "ss", remoteEndpoint} -> {kind = CLIENT, "cs" "cr", localEndpoint, remoteEndpoint}, {kind = SERVER, "sr" "ss", localEndpoint, remoteEndpoint}
  4. complete switch to v2 model
    {kind = CLIENT, "cs" "cr", localEndpoint, remoteEndpoint}, {kind = SERVER, "sr" "ss", localEndpoint, remoteEndpoint} -> {kind = CLIENT, timestamp, duration, localEndpoint, remoteEndpoint}, {kind = SERVER, timestamp, duration, localEndpoint, remoteEndpoint, shared=true}

@codefromthecrypt
Copy link
Member Author

one interesting design luck is that in the current UI we are a little inefficient. We make one traces api query for the index, then redundantly make a trace api query to show the trace view. A better UI will reuse the same data. However when we are porting logic, this is a bit lucky. Since each screen uses a different api, I can break one screen and not break the other one. For example, I can port the search screen to use v2 without touching the trace view screen.

@codefromthecrypt
Copy link
Member Author

Another thing to consider. We should have stub/mock data that includes some interesting traces:

  • trace with clock skew
  • regular RPC trace
  • messaging trace

This way we can validate the UI behavior without being data dependent, even if that is visually comparing.

codefromthecrypt pushed a commit that referenced this issue Oct 31, 2018
Simplifies summary logic by not doing skew correction
6f79321 
The trace summary screen and individual trace screens are served by
different data. At the moment, offsets are not considered when computing
the trace duration broken down by service. Maybe it should be, but
removing clock skew makes migration to v2 simpler and we can then
revisit this logic.

See #2217
@codefromthecrypt codefromthecrypt mentioned this issue Oct 31, 2018
Merged
@codefromthecrypt
Copy link
Member Author

sample data is now in. I will do the trace summary screen conversion to v2 first as it is simplest and decoupled from the trace view one

codefromthecrypt pushed a commit that referenced this issue Oct 31, 2018
@adriancole
Simplifies summary logic by not doing skew correction (#2228)
d91d73b 
The trace summary screen and individual trace screens are served by
different data. At the moment, offsets are not considered when computing
the trace duration broken down by service. Maybe it should be, but
removing clock skew makes migration to v2 simpler and we can then
revisit this logic.

See #2217
@codefromthecrypt codefromthecrypt mentioned this issue Nov 3, 2018
Merged
codefromthecrypt referenced this issue Nov 8, 2018
Implements javascript span merging for v2 format
d589ca3 
Next step is correcting clock skew in v2 format
codefromthecrypt pushed a commit that referenced this issue Dec 2, 2018
Completes refactor of UI to v2 native
27f061c 
This is the last step where the UI no longer uses any v1 types.

What it does now is a dual-pass where at first v2 spans are collected
into events such that spans that share an ID can be in the same row.

After that, it decorates them with positional details, such as where
to put annotations or how wide the span line should be.

fixes #2217
@codefromthecrypt codefromthecrypt mentioned this issue Dec 2, 2018
Merged
codefromthecrypt pushed a commit that referenced this issue Dec 2, 2018
@adriancole
Completes refactor of UI to v2 native (#2304)
788ec28 
This is the last step where the UI no longer uses any v1 types.

What it does now is a dual-pass where at first v2 spans are collected
into events such that spans that share an ID can be in the same row.

After that, it decorates them with positional details, such as where
to put annotations or how wide the span line should be.

fixes #2217
abesto pushed a commit to abesto/zipkin that referenced this issue Sep 10, 2019
@adriancole @abesto
Simplifies summary logic by not doing skew correction (openzipkin#2228)
d005ba5 
The trace summary screen and individual trace screens are served by
different data. At the moment, offsets are not considered when computing
the trace duration broken down by service. Maybe it should be, but
removing clock skew makes migration to v2 simpler and we can then
revisit this logic.

See openzipkin#2217
abesto pushed a commit to abesto/zipkin that referenced this issue Sep 10, 2019
@adriancole @abesto
Completes refactor of UI to v2 native (openzipkin#2304)
9301d55 
This is the last step where the UI no longer uses any v1 types.

What it does now is a dual-pass where at first v2 spans are collected
into events such that spans that share an ID can be in the same row.

After that, it decorates them with positional details, such as where
to put annotations or how wide the span line should be.

fixes openzipkin#2217
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement ui Zipkin UI
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Completes refactor of UI to v2 native openzipkin/zipkin
2 participants
@codefromthecrypt @GuangmingLuo