Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace tracestate header with baggage #2078

Merged
merged 27 commits into from
Jun 15, 2022

Conversation

adinauer
Copy link
Member

@adinauer adinauer commented Jun 3, 2022

📜 Description

Replace tracestate header with baggage header.

💡 Motivation and Context

Baggage (8192 chars) has a larger size limit than tracestate (256 chars).

💚 How did you test it?

Unit Tests

📝 Checklist

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

🔮 Next steps

@codecov-commenter
Copy link

codecov-commenter commented Jun 3, 2022

Codecov Report

Merging #2078 (37aab6d) into main (580aa75) will decrease coverage by 0.17%.
The diff coverage is 85.79%.

@@             Coverage Diff              @@
##               main    #2078      +/-   ##
============================================
- Coverage     81.11%   80.93%   -0.18%     
- Complexity     3232     3254      +22     
============================================
  Files           230      231       +1     
  Lines         11850    11950     +100     
  Branches       1572     1586      +14     
============================================
+ Hits           9612     9672      +60     
- Misses         1652     1698      +46     
+ Partials        586      580       -6     
Impacted Files Coverage Δ
sentry/src/main/java/io/sentry/HubAdapter.java 7.81% <0.00%> (ø)
sentry/src/main/java/io/sentry/NoOpHub.java 45.45% <ø> (ø)
...ntry/src/main/java/io/sentry/NoOpSentryClient.java 70.00% <ø> (ø)
sentry/src/main/java/io/sentry/NoOpSpan.java 25.00% <0.00%> (ø)
...entry/src/main/java/io/sentry/NoOpTransaction.java 25.80% <0.00%> (ø)
...ring/tracing/SentrySpanClientWebRequestFilter.java 67.50% <33.33%> (-2.78%) ⬇️
...n/java/io/sentry/apollo/SentryApolloInterceptor.kt 81.57% <60.00%> (-1.76%) ⬇️
sentry/src/main/java/io/sentry/BaggageHeader.java 62.50% <62.50%> (ø)
sentry/src/main/java/io/sentry/SentryTracer.java 91.17% <87.50%> (ø)
sentry/src/main/java/io/sentry/Baggage.java 89.02% <89.02%> (ø)
... and 11 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 580aa75...37aab6d. Read the comment docs.

@adinauer
Copy link
Member Author

adinauer commented Jun 3, 2022

FYI @brustolin

@adinauer
Copy link
Member Author

adinauer commented Jun 3, 2022

@AbhiPrasad and @jan-auer could you please take a quick look at the sections / comments I marked with 👀 ?

@AbhiPrasad AbhiPrasad self-requested a review June 3, 2022 15:23
@adinauer adinauer marked this pull request as ready for review June 3, 2022 15:26
@jan-auer jan-auer self-requested a review June 3, 2022 16:36
Base automatically changed from 6.x.x to main June 7, 2022 10:10
@adinauer
Copy link
Member Author

adinauer commented Jun 7, 2022

moved to code, so discussion can be more easily threaded

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

some nits only: :shipit:

sentry/src/main/java/io/sentry/Baggage.java Show resolved Hide resolved
sentry/src/main/java/io/sentry/Baggage.java Show resolved Hide resolved
sentry/src/main/java/io/sentry/Baggage.java Show resolved Hide resolved
Copy link
Member

@AbhiPrasad AbhiPrasad 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 is a good place to start!

@adinauer adinauer merged commit a88509c into main Jun 15, 2022
@adinauer adinauer deleted the feat/replace-trace-state-with-baggage branch June 15, 2022 06:22
@ApiStatus.Experimental
public final class Baggage {

static final @NotNull String CHARSET = StandardCharsets.UTF_8.toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

@adinauer this is not supported on Android.
We have to private static final Charset UTF_8 = Charset.forName("UTF-8");

Copy link
Contributor

Choose a reason for hiding this comment

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

We have to do #1403 to avoid such issues, a simple and empty project already helps, set to API 16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants