Skip to content

Make sure trace context headers are added only once - #1937

Merged
SylvainJuge merged 4 commits into
elastic:masterfrom
eyalkoren:http-span-traceparent-fix
Jul 28, 2021
Merged

Make sure trace context headers are added only once#1937
SylvainJuge merged 4 commits into
elastic:masterfrom
eyalkoren:http-span-traceparent-fix

Conversation

@eyalkoren

@eyalkoren eyalkoren commented Jul 28, 2021 •
edited by zube Bot
Loading

Copy link
Copy Markdown
Contributor

What does this PR do?

Make sure trace context headers are added only once to outgoing calls, even if the protocol supports multiple values per header (like in HTTP).

Checklist

  • This is a bugfix

@eyalkoren
eyalkoren requested a review from SylvainJuge July 28, 2021 09:58
@github-actions github-actions Bot added agent-java community Issues and PRs created by the community labels Jul 28, 2021
@ghost

ghost commented Jul 28, 2021 •
edited by ghost
Loading

Copy link
Copy Markdown

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-07-28T11:39:18.531+0000

  • Duration: 55 min 1 sec

  • Commit: 20c5f12

Test stats 🧪

Test Results
Failed 0
Passed 2357
Skipped 19
Total 2376

Trends 🧪

Image of Build Times

Image of Tests

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 2357
Skipped 19
Total 2376

SylvainJuge
SylvainJuge approved these changes Jul 28, 2021
View reviewed changes

@SylvainJuge SylvainJuge left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[minor] For HTTP headers, most (if not all) header accessors allow for ready & write operations, thus maybe we could have a helper method or a variant of propagateTraceContext that ensure that header has only a single value.

@eyalkoren

eyalkoren commented Jul 28, 2021

Copy link
Copy Markdown
Contributor Author

For HTTP headers, most (if not all) header accessors allow for ready & write operations, thus maybe we could have a helper method or a variant of propagateTraceContext that ensure that header has only a single value.

Yes, that's what I wanted to do at first, but then realized that in some cases it's not efficient to read (allocates iterator for example), so using set instead of add (i.e. override) is cheaper (even though it's not equivalent).
In Spring RestTemplate I used a containsKey API on a per-header basis.
In OkHttp you even need to use a different type for reading and writing.
So I ended up doing it case by case, at least for now.

@SylvainJuge
SylvainJuge merged commit b3a8c14 into elastic:master Jul 28, 2021
@eyalkoren
eyalkoren deleted the http-span-traceparent-fix branch July 28, 2021 15:01
@AlexanderWert AlexanderWert removed the community Issues and PRs created by the community label Aug 2, 2021
v1v added a commit to v1v/apm-agent-java that referenced this pull request Aug 2, 2021
@v1v
Merge remote-tracking branch 'upstream/master' into feature/add-java-…
ed364ff 
…junit-support

* upstream/master:
  Updated get-user-teams-membership to version 1.0.3
  ensure that the socket is closed even if there is an exception from the other close (elastic#1946)
  Make sure trace context headers are added only once (elastic#1937)
  Update CHANGELOG.asciidoc
  Ecs reformatting more fields (elastic#1910)
  Use path when high-level framework method is unknown (elastic#1906)
  Semver parsing enhancement (elastic#1931)
v1v added a commit to v1v/apm-agent-java that referenced this pull request Aug 2, 2021
@v1v
Merge remote-tracking branch 'upstream/master' into feature/pipeline-…
178844e 
…for-windows-only

* upstream/master: (100 commits)
  [CI] Enable compatibility test matrix for unit tests (elastic#1915)
  Updated get-user-teams-membership to version 1.0.3
  ensure that the socket is closed even if there is an exception from the other close (elastic#1946)
  Make sure trace context headers are added only once (elastic#1937)
  Update CHANGELOG.asciidoc
  Ecs reformatting more fields (elastic#1910)
  Use path when high-level framework method is unknown (elastic#1906)
  Semver parsing enhancement (elastic#1931)
  Bump version.slf4j from 1.7.31 to 1.7.32 (elastic#1933)
  Add description about memory pool metrics to docs (elastic#1925)
  updated team membership check in action
  Add 1.25.0 to cloudfoundry index
  Update CHANGELOG.asciidoc (elastic#1929)
  fixed community label action
  [maven-release-plugin] prepare for next development iteration
  [maven-release-plugin] prepare release v1.25.0
  Prepare release 1.25.0 (elastic#1927)
  synchronize json schema specs (elastic#1926)
  added labeling of community issues and PRs
  added community labeler config
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SylvainJuge SylvainJuge SylvainJuge approved these changes

Assignees

No one assigned

Labels

agent-java

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@eyalkoren @SylvainJuge @AlexanderWert @elastic-apm-tech