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

fix(zipkin): do not reuse propagated_span by default #10983

Merged
merged 3 commits into from
Jun 1, 2023

Conversation

ms2008
Copy link
Contributor

@ms2008 ms2008 commented Jun 1, 2023

Summary

Zipkin manages its spans and calls finish on them, and the result is that finish is called twice if instrumentations are enabled. Then an assertion error is generated, resulting in the span not being sent correctly. This was an issue introduced with #10663

So my approach is not to store the propagation span if the reuse parameter is not specified, in order to avoid the traces not being generated correctly under that.

Checklist

Full changelog

  • do not reuse propagated_span by default

Issue reference

Fix FTI-5094

Zipkin manages its spans and calls `finish` on them, and the result is
that `finish` is called twice if instrumentations are enabled. Then an
assertion error is generated, resulting in the span not being sent
correctly. This was an issue introduced with
#10663

So my approach is not to store the propagation span if the reuse
parameter is not specified, in order to avoid the traces not being
generated correctly under that.
@ms2008 ms2008 force-pushed the fix/zipkin-proxy_span-reuse branch from 4a7da84 to ee4feb5 Compare June 1, 2023 06:59
@ms2008 ms2008 marked this pull request as ready for review June 1, 2023 08:13
@ms2008 ms2008 requested a review from samugi June 1, 2023 08:13
Copy link
Member

@samugi samugi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good job thank you @ms2008, just a couple of optional suggestions to rename the test descriptions

spec/03-plugins/34-zipkin/zipkin_spec.lua Outdated Show resolved Hide resolved
spec/03-plugins/34-zipkin/zipkin_spec.lua Outdated Show resolved Hide resolved
@samugi samugi merged commit bb93aa7 into master Jun 1, 2023
@samugi samugi deleted the fix/zipkin-proxy_span-reuse branch June 1, 2023 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants