Updating Trace Context headers implementation by eyalkoren · Pull Request #1024 · elastic/apm-agent-java

eyalkoren · 2020-02-06T09:34:13Z

What does this PR do?

Implementing

Implementing elastic/apm#71. Since this logic touches LOTs of plugins for both server and client sides of the tracing, it starts with a refactor that adds new APIs so that all plugins are indifferent to the logic of headers additions/reads/removal.

Checklist

My code follows the style guidelines of this project
I have rebased my changes on top of the latest master branch

I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

I have updated CHANGELOG.asciidoc

Author's Checklist

Related issues

Closes #923

Use cases

Screenshots

…entation

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/ElasticApmTracer.java

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/HeaderGetter.java

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/TraceContext.java

felixbarny · 2020-02-06T10:14:20Z

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/TraceContext.java

+        }
+        byte[] buffer = headerSetter.getFixedLengthByteArray(TRACE_PARENT_BINARY_HEADER_NAME, BINARY_FORMAT_EXPECTED_LENGTH);
+        if (buffer == null || buffer.length != BINARY_FORMAT_EXPECTED_LENGTH) {
+            logger.warn("Header setter {} failed to provide a byte buffer with the proper length. Allocating a buffer for each header.",


There's a chance that this will be logged very frequently. Do any of the existing implementations return null?

There is a single implementation (Kafka) that returns null if the current assumption (of same thread setting and serializing the headers) becomes invalid. If and when that happens, I don't mind the first user encountering it will get lots of errors. I made it so that distributed tracing will continue to work, but will start allocating more than before, so it can be easily overlooked if not logged clearly.

We should track https://issues.apache.org/jira/browse/KAFKA-3539

codecov-io · 2020-02-06T11:31:11Z

Codecov Report

❗ No coverage uploaded for pull request base (master@83d5248). Click here to learn what that means.
The diff coverage is 58.62%.

@@            Coverage Diff            @@
##             master    #1024   +/-   ##
=========================================
  Coverage          ?   61.69%           
  Complexity        ?       85           
=========================================
  Files             ?      288           
  Lines             ?    11298           
  Branches          ?     1499           
=========================================
  Hits              ?     6970           
  Misses            ?     3881           
  Partials          ?      447

Impacted Files	Coverage Δ	Complexity Δ
...va/co/elastic/apm/agent/impl/ElasticApmTracer.java	`68.75% <ø> (ø)`	`0 <0> (?)`
...java/co/elastic/apm/agent/bci/ElasticApmAgent.java	`72.72% <75%> (ø)`	`0 <0> (?)`
...astic/apm/agent/jdbc/StatementInstrumentation.java	`30.14% <8.69%> (ø)`	`0 <0> (?)`
...client/AbstractAsyncHttpClientInstrumentation.java	`44.26% <87.5%> (ø)`	`0 <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 83d5248...2bf09df. Read the comment docs.

Improvements - MethodHandles are not @nullable - Both getFirstHeader and getAllHeaders method handles are used in HeadersExtractorBridge

Allows for internal iteration which can be more efficient - No forced Iterator allocations - No conversion from Header to String/byte[] necessary - Consumer is stateless and can be re-used, due to state method argument

felixbarny

I made some suggestions in eyalkoren#1

See also the messages of the individual commits.

…a into felixbarny-pull/1024

robgil · 2020-02-09T09:00:51Z

Codecov Report

Merging #1024 into master will not change coverage by %.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #1024   +/-   ##
=========================================
  Coverage     62.30%   62.30%           
  Complexity       85       85           
=========================================
  Files           290      290           
  Lines         11375    11375           
  Branches       1514     1514           
=========================================
  Hits           7087     7087           
  Misses         3840     3840           
  Partials        448      448

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 d724774...c284fc4. Read the comment docs.

…entation

apm-agent-core/src/main/java/co/elastic/apm/agent/configuration/CoreConfiguration.java

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/HeaderGetter.java

felixbarny · 2020-02-10T08:06:33Z

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/TraceContext.java

+        }
+        byte[] buffer = headerSetter.getFixedLengthByteArray(TRACE_PARENT_BINARY_HEADER_NAME, BINARY_FORMAT_EXPECTED_LENGTH);
+        if (buffer == null || buffer.length != BINARY_FORMAT_EXPECTED_LENGTH) {
+            logger.warn("Header setter {} failed to provide a byte buffer with the proper length. Allocating a buffer for each header.",


We should track https://issues.apache.org/jira/browse/KAFKA-3539

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/TextTracestateAppender.java

apm-agent-core/src/test/java/co/elastic/apm/agent/impl/BinaryHeaderMapAccessor.java

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/TraceContext.java

felixbarny · 2020-02-10T08:52:42Z

...ins/apm-api-plugin/src/main/java/co/elastic/apm/agent/plugin/api/HeadersExtractorBridge.java

+        private Extractor() {}

-    private HeadersExtractorBridge() {
+        public static Extractor of(@Nullable Object headerExtractor, Object headersExtractor) {


This does not work, unfortunately. There can be multiple implementations passed to the API. Even for the same technology, the headerExtractor will be specific to a particular instance of a request object.

Of course 🤦‍♂
I can make it ThreadLocal, or cache per extractor. Since it's our API implementation, I want to make sure it is in accordance to the rest of the stuff we do

let's just allocate in this case. There will be allocations anyway for a capturing lambda (like request::getHeaders) and the header iterator. Allocations are fine and inevitable in the public API.

…uilder

…tation

Refactoring- adding header set and get APIs

83d5248

eyalkoren requested a review from felixbarny February 6, 2020 09:34

eyalkoren added 2 commits February 6, 2020 12:03

Merge remote-tracking branch 'upstream/master' into tracestate-implem…

e19351a

…entation

Merging AsyncHttpClient

519a2c6

felixbarny reviewed Feb 6, 2020

View reviewed changes

Generics enhancements

2bf09df

felixbarny added 2 commits February 7, 2020 10:20

More generics

66058db

Suggestion to improve Header*Bridge

66bc6a2

Improvements - MethodHandles are not @nullable - Both getFirstHeader and getAllHeaders method handles are used in HeadersExtractorBridge

felixbarny mentioned this pull request Feb 7, 2020

Review suggestions eyalkoren/apm-agent-java#1

Merged

Remove HeaderGetter#getHeaders in favor of HeaderGetter#forEach

0433c7b

Allows for internal iteration which can be more efficient - No forced Iterator allocations - No conversion from Header to String/byte[] necessary - Consumer is stateless and can be re-used, due to state method argument

felixbarny reviewed Feb 7, 2020

View reviewed changes

eyalkoren added 2 commits February 9, 2020 10:44

Apply review suggestions

d90e10b

Merge branch 'pull/1024' of git://github.com/felixbarny/apm-agent-jav…

a0a86f9

…a into felixbarny-pull/1024

eyalkoren added 5 commits February 9, 2020 11:23

minor adjustments

31e7b84

Merge branch 'felixbarny-pull/1024' into tracestate-implementation

32bfb62

Implement usage of the W3C traceparent header

c743932

Update docs

2629e09

Implement tracestate header

95719a6

eyalkoren changed the title ~~[WIP] Updating Trace Context headers implementation~~ Updating Trace Context headers implementation Feb 10, 2020

eyalkoren requested a review from felixbarny February 10, 2020 04:56

eyalkoren added 3 commits February 10, 2020 09:52

Remove binary header when requested

6327508

Remove debug prints

e11b568

Merge remote-tracking branch 'upstream/master' into tracestate-implem…

c284fc4

…entation

felixbarny requested changes Feb 10, 2020

View reviewed changes

felixbarny added 2 commits February 13, 2020 17:37

Add tracestate to ArrayList(1) instead of appending to pooled StringB…

d62f17c

…uilder

Fix HeadersExtractorBridge.Extractor

568c15d

felixbarny approved these changes Feb 14, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into tracestate-implemen…

c509c3e

…tation

Add changelog, adjust added version to 1.14.0

756ad07

felixbarny merged commit 9528d35 into elastic:master Feb 14, 2020

eyalkoren deleted the tracestate-implementation branch August 3, 2020 13:30

Conversation

eyalkoren commented Feb 6, 2020 • edited by zube bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Checklist

Author's Checklist

Related issues

Use cases

Screenshots

Uh oh!

Uh oh!

Uh oh!

Uh oh!

felixbarny Feb 6, 2020

Choose a reason for hiding this comment

Uh oh!

eyalkoren Feb 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixbarny Feb 10, 2020

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Feb 6, 2020

Codecov Report

Uh oh!

felixbarny left a comment

Choose a reason for hiding this comment

Uh oh!

robgil commented Feb 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

felixbarny Feb 10, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

felixbarny Feb 10, 2020

Choose a reason for hiding this comment

Uh oh!

eyalkoren Feb 10, 2020

Choose a reason for hiding this comment

Uh oh!

felixbarny Feb 13, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

eyalkoren commented Feb 6, 2020 •

edited by zube bot

Loading

eyalkoren Feb 9, 2020 •

edited

Loading

robgil commented Feb 9, 2020 •

edited

Loading