Feat: Add `tracestate` HTTP header support. #1683

maciejwalkowiak · 2021-08-25T11:04:25Z

📜 Description

Add experimental tracestate HTTP header support defined in https://develop.sentry.dev/sdk/performance/trace-context/#client-options.

Trace context is added to each envelope containing transaction or an event.

In addition to that, SentryOkHttpInterceptor attaches a new tracestate header to all outgoing requests.

By default this feature is off and is controller by a traceSampling flag on SentryOptions. All new API classes or methods are marked as @Experimental.

💚 How did you test it?

Unit & Integration tests. Sending events to sentry-java project on in sentry-sdks and verifying if they appear.

📝 Checklist

I reviewed the submitted code
I added tests to verify the changes
I updated the docs if needed
No breaking changes

sentry/src/main/java/io/sentry/Hub.java

codecov-commenter · 2021-08-25T15:50:00Z

Codecov Report

Merging #1683 (e66d857) into main (90a6821) will decrease coverage by 1.10%.
The diff coverage is 54.06%.

@@             Coverage Diff              @@
##               main    #1683      +/-   ##
============================================
- Coverage     75.93%   74.82%   -1.11%     
- Complexity     2025     2078      +53     
============================================
  Files           207      210       +3     
  Lines          7060     7428     +368     
  Branches        699      782      +83     
============================================
+ Hits           5361     5558     +197     
- Misses         1361     1488     +127     
- Partials        338      382      +44

Impacted Files	Coverage Δ
sentry/src/main/java/io/sentry/HubAdapter.java	`5.08% <0.00%> (ø)`
sentry/src/main/java/io/sentry/NoOpHub.java	`47.50% <ø> (ø)`
...ntry/src/main/java/io/sentry/NoOpSentryClient.java	`60.00% <ø> (ø)`
sentry/src/main/java/io/sentry/NoOpSpan.java	`27.27% <0.00%> (-2.73%)`	⬇️
...entry/src/main/java/io/sentry/NoOpTransaction.java	`19.35% <0.00%> (-1.34%)`	⬇️
sentry/src/main/java/io/sentry/vendor/Base64.java	`31.94% <31.94%> (ø)`
sentry/src/main/java/io/sentry/IHub.java	`86.36% <50.00%> (-4.12%)`	⬇️
sentry/src/main/java/io/sentry/ISentryClient.java	`85.71% <66.66%> (-3.76%)`	⬇️
...ntry/src/main/java/io/sentry/TraceStateHeader.java	`80.00% <80.00%> (ø)`
...in/java/io/sentry/SentryEnvelopeHeaderAdapter.java	`79.60% <82.53%> (+1.82%)`	⬆️
... and 12 more

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 90a6821...e66d857. Read the comment docs.

sentry/src/main/java/io/sentry/SentryClient.java

sentry/src/main/java/io/sentry/SentryEnvelopeHeaderAdapter.java

marandaneto · 2021-08-26T07:45:09Z

ps: important to test the serialization and deserialization of an envelope without such information and guarantee that nothing is broken since users that do offline caching might experience the broken behavior, if any

…a Span.

maciejwalkowiak · 2021-08-26T12:29:32Z

To ensure that the state header for a transaction does not change once its calculated once, I moved computing the header from capturing transaction/event, to the SentryTracer, where it can be computed only once.

bruno-garcia

Please make it opt-in, aligning with option named here: https://develop.sentry.dev/sdk/trace-context/

Please please add a huge javadoc to the option that this is experimental and will be removed without notice.

bruno-garcia · 2021-08-27T20:01:16Z

@marandaneto before merging this could you help @maciejwalkowiak test the OkHttp bit? He'll setup the sentry-sdks org, sentry-java repo with some rules and we can test things out:

https://sentry.io/settings/sentry-sdks/projects/sentry-java/filters-and-sampling/

bruno-garcia

Couple notes but lets merge and make a release of this please.
Lets make a beta first please

sentry/src/main/java/io/sentry/ISpan.java

bruno-garcia · 2021-08-31T21:56:15Z

sentry/src/main/java/io/sentry/SentryTracer.java

+    if (hub.getOptions().isTraceSampling()) {
+      if (traceState == null) {
+        final AtomicReference<User> userAtomicReference = new AtomicReference<>();
+        hub.configureScope(
+            scope -> {
+              userAtomicReference.set(scope.getUser());
+            });
+        this.traceState = new TraceState(this, userAtomicReference.get(), hub.getOptions());
+      }
+      return this.traceState;
+    } else {
+      return null;
+    }


If the trace gets passed around to different threads so they can create child spans, this could have concurrent access, right?

If so, what about a double check lock?

Suggested change

if (hub.getOptions().isTraceSampling()) {

if (traceState == null) {

final AtomicReference<User> userAtomicReference = new AtomicReference<>();

hub.configureScope(

scope -> {

userAtomicReference.set(scope.getUser());

});

this.traceState = new TraceState(this, userAtomicReference.get(), hub.getOptions());

}

return this.traceState;

} else {

return null;

}

if (hub.getOptions().isTraceSampling()) {

if (traceState == null) {

synchronized(this) {

if (traceState == null) {

final AtomicReference<User> userAtomicReference = new AtomicReference<>();

hub.configureScope(

scope -> {

userAtomicReference.set(scope.getUser());

});

this.traceState = new TraceState(this, userAtomicReference.get(), hub.getOptions());

}

}

}

}

return this.traceState;

} else {

return null;

}

bruno-garcia · 2021-08-31T23:50:16Z

sentry/src/main/java/io/sentry/SentryTracer.java

+  }
+
+  @Override
+  public @Nullable TraceStateHeader toTraceStateHeader() {


Since TraceState has a reference to options, could we have toTraceStateHeader be part of TraceState instead and avoid this method?

TraceState does not have a reference to options. We can put it to a transient field, just somehow does not seem right to me as TraceState was meant to be just a data container.

That's a fair point. I'm just trying to find any way not to add these two public methods here but I'm about to lose hope (the PR is approved anyway so)

SentryTracer is @ApiStatus.Internal already, so it'd be fine to be there.

bruno-garcia · 2021-09-01T20:43:02Z

@marandaneto we need to ship soon, would you like to give this a pass before we merge?

marandaneto · 2021-09-02T08:15:52Z

sentry/src/main/java/io/sentry/SentryOptions.java

@@ -280,6 +280,9 @@
   */
  private @NotNull RequestSize maxRequestBodySize = RequestSize.NONE;

+  /** Controls if the `tracestate` header is attached to envelopes and HTTP client integrations. */
+  private boolean traceSampling;


is it disabled by default?
if this is intended for mobile too, let's add the ability to turn it on via Android Manifest
See: ManifestMetadataReader

marandaneto · 2021-09-02T08:20:34Z

sentry/src/main/java/io/sentry/SentryTracer.java

+  public @Nullable TraceState traceState() {
+    if (hub.getOptions().isTraceSampling()) {
+      if (traceState == null) {
+        final AtomicReference<User> userAtomicReference = new AtomicReference<>();


what's about adding a function to the Scope called withUser like we do with withSession instead of using AtomicReference? that executes a callback and returns the user atomically, it could be package private or @ApiStatus.Internal. not a big fun of AtomicReference

I agree, also didn't like it, but in this PR we need to avoid introducing changes to the API, so i think AtomicReference as a temporary solution is a right trade off.

maciejwalkowiak added 2 commits August 25, 2021 13:04

Feat: Add tracestate HTTP header support.

4c3e590

Polish.

bf9cb7e

bruno-garcia reviewed Aug 25, 2021

View reviewed changes

sentry/src/main/java/io/sentry/Hub.java Outdated Show resolved Hide resolved

Remove method from Sentry class 🤦‍♂️

5bc7e25

brustolin reviewed Aug 25, 2021

View reviewed changes

sentry/src/main/java/io/sentry/SentryClient.java Outdated Show resolved Hide resolved

Create trace state when scope is null

944cf8c

marandaneto reviewed Aug 26, 2021

View reviewed changes

sentry/src/main/java/io/sentry/SentryEnvelopeHeaderAdapter.java Outdated Show resolved Hide resolved

Remove method from Hub, add a method to create trace state header to …

4dc6382

…a Span.

maciejwalkowiak added 8 commits August 27, 2021 10:19

Fix Base64 encoding.

96762db

Add tests for SentryOkHttpInterceptor.

62463d2

Docs & tests

a869202

Add trace state serialization & deserialization

8e7c823

Fix user serialization.

8a9b419

Add SentryClient tests.

e37069c

Add tests.

06d262f

Merge remote-tracking branch 'origin/main' into tracestate-header

e5149a8

bruno-garcia reviewed Aug 27, 2021

View reviewed changes

bruno-garcia mentioned this pull request Aug 27, 2021

feat: Add tracestate HTTP header support getsentry/sentry-cocoa#1291

Merged

5 tasks

maciejwalkowiak added 3 commits August 30, 2021 13:25

Revert API backward incompatible change, add Javadocs.

ea40dda

Add "traceSampling" flag.

4e140f5

Fix tests.

d551922

maciejwalkowiak marked this pull request as ready for review August 30, 2021 13:40

Changelog.

86e5526

bruno-garcia requested a review from marandaneto August 30, 2021 14:07

bruno-garcia approved these changes Aug 31, 2021

View reviewed changes

bruno-garcia reviewed Aug 31, 2021

View reviewed changes

marandaneto reviewed Sep 2, 2021

View reviewed changes

maciejwalkowiak added 3 commits September 2, 2021 10:25

Fix concurrency issues in SentryTrace#traceState.

2f0762c

Read traceSampling from Android manifest.

fe589be

Merge remote-tracking branch 'origin/main' into tracestate-header

e66d857

marandaneto approved these changes Sep 2, 2021

View reviewed changes

maciejwalkowiak merged commit 8dcd504 into main Sep 2, 2021

maciejwalkowiak deleted the tracestate-header branch September 2, 2021 09:18

marandaneto mentioned this pull request Sep 2, 2021

Add allowList for the tracestate header #1697

Closed

marandaneto mentioned this pull request May 20, 2022

Replace trace state HTTP headers with baggage getsentry/team-mobile#15

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Add `tracestate` HTTP header support. #1683

Feat: Add `tracestate` HTTP header support. #1683

maciejwalkowiak commented Aug 25, 2021 •

edited

Loading

codecov-commenter commented Aug 25, 2021 •

edited

Loading

marandaneto commented Aug 26, 2021

maciejwalkowiak commented Aug 26, 2021

bruno-garcia left a comment

bruno-garcia commented Aug 27, 2021

bruno-garcia left a comment

bruno-garcia Aug 31, 2021

maciejwalkowiak Sep 2, 2021

bruno-garcia Aug 31, 2021 •

edited

Loading

maciejwalkowiak Sep 1, 2021

bruno-garcia Sep 1, 2021

marandaneto Sep 2, 2021

bruno-garcia commented Sep 1, 2021

marandaneto Sep 2, 2021

maciejwalkowiak Sep 2, 2021

marandaneto Sep 2, 2021

maciejwalkowiak Sep 2, 2021

Feat: Add tracestate HTTP header support. #1683

Feat: Add tracestate HTTP header support. #1683

Conversation

maciejwalkowiak commented Aug 25, 2021 • edited Loading

📜 Description

💚 How did you test it?

📝 Checklist

codecov-commenter commented Aug 25, 2021 • edited Loading

Codecov Report

marandaneto commented Aug 26, 2021

maciejwalkowiak commented Aug 26, 2021

bruno-garcia left a comment

Choose a reason for hiding this comment

bruno-garcia commented Aug 27, 2021

bruno-garcia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruno-garcia Aug 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruno-garcia commented Sep 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Feat: Add `tracestate` HTTP header support. #1683

Feat: Add `tracestate` HTTP header support. #1683

maciejwalkowiak commented Aug 25, 2021 •

edited

Loading

codecov-commenter commented Aug 25, 2021 •

edited

Loading

bruno-garcia Aug 31, 2021 •

edited

Loading