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.

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

Simplifies summary logic by not doing skew correction #2228

Merged
merged 1 commit into from
Oct 31, 2018

Conversation

codefromthecrypt
Copy link
Member

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

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
Copy link
Member Author

To test this, I used "skew.json" before and after the change and noticed the service percentage was exactly the same. This is how I know the percentage function doesn't consider offset even without looking at the code (I also looked at the code though!)

screen shot 2018-10-31 at 11 26 44 am

@codefromthecrypt codefromthecrypt merged commit d91d73b into master Oct 31, 2018
@tacigar
Copy link
Member

tacigar commented Oct 31, 2018

I think that is good!
But I have a question.

I think that adjustTimestamps in skew.js may change the timestamps of the spans, and traceDuration in traceSummary.js will be used for duration calculation.
And I think that in many case traceDuration function returns span[0].timestamp + span[0].duration - span[0].timestamp (== span[0].duration).
So in many case, I think the process of clock-skew is unnecessary.
Is my opinion correct?
And is this even in all cases?

Sorry for newbie question 😞

@codefromthecrypt codefromthecrypt deleted the dont-skew-correct-summary branch October 31, 2018 03:27
@codefromthecrypt
Copy link
Member Author

This may indeed be a problem. I will look into it now as we don't have data yet to break this.

codefromthecrypt pushed a commit that referenced this pull request Oct 31, 2018
In #2228 my assumption was incorrect as @tacigar highlighted. This makes
a test to ensure the logic stays the same for now.
codefromthecrypt pushed a commit that referenced this pull request Oct 31, 2018
#2229)

In #2228 my assumption was incorrect as @tacigar highlighted. This makes
a test to ensure the logic stays the same for now.
abesto pushed a commit to abesto/zipkin that referenced this pull request Sep 10, 2019
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 pull request Sep 10, 2019
openzipkin#2229)

In openzipkin#2228 my assumption was incorrect as @tacigar highlighted. This makes
a test to ensure the logic stays the same for now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore ui Zipkin UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants