opentelemetry-exporter-richconsole: prevent deadlock #3900
Conversation
| while spans: | ||
| for span in spans: | ||
| if not span.parent: | ||
| if not span.parent or span.parent.span_id not in span_ids: |
There was a problem hiding this comment.
Make sure that the parent is part of the spans. If not, treat the child span as root span.
| ) | ||
| trees[trace_id] = Tree(label=f"Trace {trace_id}") | ||
| child = trees[trace_id].add( | ||
| tree = trees.setdefault(trace_id, Tree(label=f"Trace {trace_id}")) |
There was a problem hiding this comment.
Avoid recreating the trace tree as now we can enter this branch for multiple spans.
| for child in trees[traceid_1].children[0].children | ||
| ) | ||
|
|
||
| def test_no_deadlock(tracer_provider): |
There was a problem hiding this comment.
This is indeed a non-regression test. However, if you run this with the old version, you will get a deadlock and CI etc. will hang. If that is not acceptable, we should add a timeout mechanism, e.g. pytest-timeout.
There was a problem hiding this comment.
Yeah, a quick timeout instead of the longer GH timeout would be nice
There was a problem hiding this comment.
Are you ok with me adding pytest-timeout as test dependency?
There was a problem hiding this comment.
However, if you run this with the old version, you will get a deadlock and CI etc. will hang. If that is not acceptable
Why wouldn't it be acceptable? Seems OK to me
xrmx
changed the title
|
|
||
| ### Fixed | ||
|
|
||
| - `opentelemetry-instrumentation-richconsole`: Prevent deadlock when parent span is not part of the batch |
There was a problem hiding this comment.
| - `opentelemetry-instrumentation-richconsole`: Prevent deadlock when parent span is not part of the batch | |
| - `opentelemetry-exporter-richconsole`: Prevent deadlock when parent span is not part of the batch |
| for child in trees[traceid_1].children[0].children | ||
| ) | ||
|
|
||
| def test_no_deadlock(tracer_provider): |
There was a problem hiding this comment.
However, if you run this with the old version, you will get a deadlock and CI etc. will hang. If that is not acceptable
Why wouldn't it be acceptable? Seems OK to me
3 participants
Description
Fixes #3254
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.