Skip to content
This repository was archived by the owner on Jul 1, 2022. It is now read-only.

Conversation

@objectiser
Copy link
Contributor

Signed-off-by: Gary Brown gary@brownuk.com

Which problem is this PR solving?

If a span is created with multiple "FOLLOWS_FROM" references from different traces, the new span is created as a child of the first supplied reference. As discussed on the bi-weekly call 18th Oct, this situation should result in a new trace being created.

Short description of the changes

If more than one reference is supplied, with no CHILD_OF type, then check whether they belong to the same trace instance. If not, then treat as a root span of a new trace.

Signed-off-by: Gary Brown <gary@brownuk.com>
@codecov
Copy link

codecov bot commented Oct 20, 2019

Codecov Report

Merging #665 into master will decrease coverage by 0.24%.
The diff coverage is 96.29%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #665      +/-   ##
============================================
- Coverage     89.93%   89.68%   -0.25%     
- Complexity      569      570       +1     
============================================
  Files            69       69              
  Lines          2086     2104      +18     
  Branches        266      273       +7     
============================================
+ Hits           1876     1887      +11     
- Misses          129      135       +6     
- Partials         81       82       +1
Impacted Files Coverage Δ Complexity Δ
.../src/main/java/io/jaegertracing/Configuration.java 93.3% <100%> (+0.12%) 46 <1> (+2) ⬆️
...n/java/io/jaegertracing/internal/JaegerTracer.java 89.4% <95.45%> (+0.47%) 27 <1> (+1) ⬆️
...rtracing/internal/reporters/CompositeReporter.java 71.42% <0%> (-28.58%) 6% <0%> (-1%)
...gertracing/internal/reporters/LoggingReporter.java 81.81% <0%> (-9.1%) 4% <0%> (-1%)
...egertracing/internal/reporters/RemoteReporter.java 87.35% <0%> (-2.3%) 7% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46798bd...961d146. Read the comment docs.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

I think this needs a config option to enable new behavior, as it's not backwards compatible.

@objectiser
Copy link
Contributor Author

@yurishkuro Unless we consider the current behaviour a bug :)

There are no unit tests that clearly define what behaviour is expected in such situations - and currently there is no way to create such a root span with references to multiple other traces, which to me is a limitation/bug?

@yurishkuro
Copy link
Member

yurishkuro commented Oct 20, 2019

I don't think it's a bug. It's pretty consistent across clients and fairly reasonable - first reference becomes the parent.

The reason why a root span cannot be created is not because it's a big, but because the OT is missing a start option for that, leaving the behavior up to the tracer.

I find using first ref as parent to be more intuitive behavior than flipping to root if refs are for different traces, and it's already the current behavior, so a config option is a cleaner way imo.

Btw we should open a tracking issue in the main repo if we want to make this change across all clients.

Signed-off-by: Gary Brown <gary@brownuk.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants