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

tracing: stopping Zipkin tracing causes deadlock #30576

Closed
nvanbenschoten opened this issue Sep 24, 2018 · 1 comment · Fixed by #33287
Closed

tracing: stopping Zipkin tracing causes deadlock #30576

nvanbenschoten opened this issue Sep 24, 2018 · 1 comment · Fixed by #33287
Assignees
@nvanbenschoten
Labels
A-tracing Relating to tracing in CockroachDB. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Milestone
2.1

Comments

@nvanbenschoten
Copy link
Member

Setting a zipkin collector and then unsetting it with set cluster setting trace.zipkin.collector=''; currently deadlocks the database. All goroutines end up stuck in zipkin-go-opentracing.(*HTTPCollector).Collect+0x42.

I have a patch out to fix this here: openzipkin-contrib/zipkin-go-opentracing#113. We should make sure that patch goes in before 2.1 and should backport it to 2.0.

This also reveals a gap in our testing which we should plug once that fix goes in.
@nvanbenschoten nvanbenschoten added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-tracing Relating to tracing in CockroachDB. labels Sep 24, 2018
@nvanbenschoten nvanbenschoten added this to the 2.1 milestone Sep 24, 2018
@nvanbenschoten nvanbenschoten self-assigned this Sep 24, 2018
@nvanbenschoten
Copy link
Member Author

openzipkin-contrib/zipkin-go-opentracing#113 was merged, but we'll have to wait for the next release of zipkin-go-opentracing.

nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Dec 19, 2018
@nvanbenschoten
vendor: update thrift and zipkin-go-opentracing
0c22865 
Fixes cockroachdb#30576.
Informs cockroachdb#30774.

openzipkin-contrib/zipkin-go-opentracing@4c9fbcb...f0f479a
apache/thrift@327ebb6...2b7365c

I audited every commit in the zipkin-go-opentracing diff and every
commit to the `lib/go` package of the thrift diff.

Release note (bug fix): Updated Zipkin library to avoid deadlock when
stopping Zipkin tracing.
@nvanbenschoten nvanbenschoten mentioned this issue Dec 19, 2018
Merged
craig bot pushed a commit that referenced this issue Dec 19, 2018
@nvanbenschoten
Merge #33287
1c5df55 
33287: vendor: update thrift and zipkin-go-opentracing r=nvanbenschoten a=nvanbenschoten

Fixes #30576.
Informs #30774.

openzipkin-contrib/zipkin-go-opentracing@4c9fbcb...f0f479a
apache/thrift@327ebb6...2b7365c

I audited every commit in the zipkin-go-opentracing diff and every
commit to the `lib/go` package of the thrift diff.

Release note (bug fix): Updated Zipkin library to avoid deadlock when
stopping Zipkin tracing.

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
@craig craig bot closed this as completed in #33287 Dec 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nvanbenschoten nvanbenschoten

Labels
A-tracing Relating to tracing in CockroachDB. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Milestone
2.1
Development

Successfully merging a pull request may close this issue.

vendor: update thrift and zipkin-go-opentracing nvanbenschoten/cockroach
1 participant
@nvanbenschoten