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

Save baggage in span #189

Merged
merged 3 commits into from
Jun 28, 2017
Merged

Save baggage in span #189

merged 3 commits into from
Jun 28, 2017

Conversation

black-adder
Copy link
Contributor

@black-adder black-adder commented May 24, 2017

@codecov-io
Copy link

codecov-io commented May 24, 2017

Codecov Report

Merging #189 into master will increase coverage by 0.2%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master     #189     +/-   ##
===========================================
+ Coverage     80.22%   80.43%   +0.2%     
- Complexity      473      476      +3     
===========================================
  Files            79       79             
  Lines          1846     1855      +9     
  Branches        216      218      +2     
===========================================
+ Hits           1481     1492     +11     
+ Misses          274      273      -1     
+ Partials         91       90      -1
Impacted Files Coverage Δ Complexity Δ
...aeger-core/src/main/java/com/uber/jaeger/Span.java 84.9% <100%> (+3.46%) 39 <0> (+3) ⬆️

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 d251c18...285284f. 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.

Do we have a parent issue in uber/Jaeger repo for this? There was some rationale behind this functionality, we need to record it and link all PRs to that issue.

io.opentracing.Span parent2 = tracer.buildSpan("foo").start();
parent2.setBaggageItem("foo2", "bar");
this.assertBaggageLogs(parent2, "foo2", "bar");
Copy link
Member

@yurishkuro yurishkuro May 25, 2017

Choose a reason for hiding this comment

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

That makes me think, should we also be recording new vs override of the baggage? We cannot reliably infer that from the full trace because the parent might set the baggage after the child has been started.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't follow

Copy link
Member

Choose a reason for hiding this comment

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

iphone's autocorrect. I edited the question. When both parent and child spans set the same baggage key, we don't record if the child overrides the parent or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add fields.put("overwrite", True) to signify if we're doing a parent override.

@black-adder
Copy link
Contributor Author

Started a github project for it: https://github.com/uber/jaeger/projects/1

@@ -117,7 +117,18 @@ public String getOperationName() {
@Override
public Span setBaggageItem(String key, String value) {
synchronized (this) {
String prevItem = this.getBaggageItem(key);
this.context = this.context.withBaggageItem(key, value);
Copy link
Member

Choose a reason for hiding this comment

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

add TODO to emit a metric

This pull request was closed.
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.

3 participants