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.

Sign up for GitHub

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

Jump to bottom

Implementation for server-side baggage support and third party header handling #2226

Merged
merged 15 commits into from Sep 23, 2022
Merged

Implementation for server-side baggage support and third party header handling #2226

merged 15 commits into from Sep 23, 2022

Conversation

lbloder
Copy link
Collaborator

@lbloder lbloder commented Aug 29, 2022 •
edited

Implements server side baggage support and handling of third party baggage headers

📜 Description

💡 Motivation and Context

Fixes #2085

💚 How did you test it?

📝 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 Sep 6, 2022 •
edited

Codecov Report

Base: 80.62% // Head: 80.53% // Decreases project coverage by -0.09% ⚠️

Coverage data is based on head (e4e169b) compared to base (1e4690d).
Patch coverage: 78.14% of modified lines in pull request are covered.

Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #2226      +/-   ##
============================================
- Coverage     80.62%   80.53%   -0.10%     
- Complexity     3368     3393      +25     
============================================
  Files           240      240              
  Lines         12388    12461      +73     
  Branches       1646     1659      +13     
============================================
+ Hits           9988    10035      +47     
- Misses         1791     1813      +22     
- Partials        609      613       +4
Impacted Files Coverage Δ
...n/java/io/sentry/apollo/SentryApolloInterceptor.kt 81.57% <0.00%> (ø)
...racing/SentrySpanClientHttpRequestInterceptor.java 0.00% <0.00%> (ø)
sentry/src/main/java/io/sentry/NoOpSpan.java 25.00% <ø> (ø)
...entry/src/main/java/io/sentry/NoOpTransaction.java 25.71% <ø> (ø)
sentry/src/main/java/io/sentry/TraceContext.java 82.83% <ø> (-0.60%) ⬇️
...ring/tracing/SentrySpanClientWebRequestFilter.java 63.63% <33.33%> (-3.87%) ⬇️
...ry/src/main/java/io/sentry/TransactionContext.java 60.78% <46.66%> (-14.22%) ⬇️
sentry/src/main/java/io/sentry/BaggageHeader.java 81.81% <75.00%> (+19.31%) ⬆️
sentry/src/main/java/io/sentry/Baggage.java 83.22% <81.18%> (-6.20%) ⬇️
sentry/src/main/java/io/sentry/SentryTracer.java 90.82% <85.00%> (-0.66%) ⬇️
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

adinauer
adinauer reviewed Sep 9, 2022
View reviewed changes
Copy link
Member

@adinauer adinauer left a comment

Choose a reason for hiding this comment

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

Mostly looks good, need some more time to think about certain parts.

sentry-android-okhttp/src/test/java/io/sentry/android/okhttp/SentryOkHttpInterceptorTest.kt Outdated Show resolved Hide resolved
sentry-apollo-3/src/main/java/io/sentry/apollo3/SentryApollo3HttpInterceptor.kt Outdated Show resolved Hide resolved
sentry-apollo-3/src/test/java/io/sentry/apollo3/SentryApollo3InterceptorTest.kt Show resolved Hide resolved
sentry-openfeign/src/test/kotlin/io/sentry/openfeign/SentryFeignClientTest.kt Outdated Show resolved Hide resolved
...g-boot-starter/src/test/kotlin/io/sentry/spring/boot/SentrySpanRestTemplateCustomizerTest.kt Show resolved Hide resolved
sentry-openfeign/src/test/kotlin/io/sentry/openfeign/SentryFeignClientTest.kt Outdated Show resolved Hide resolved
sentry/api/sentry.api Show resolved Hide resolved
sentry/src/main/java/io/sentry/Baggage.java Show resolved Hide resolved
sentry/src/main/java/io/sentry/SentryTracer.java Show resolved Hide resolved
sentry/src/main/java/io/sentry/Baggage.java Show resolved Hide resolved
lbloder and others added 2 commits September 11, 2022 11:18
@lbloder @adinauer
Update sentry-android-okhttp/src/test/java/io/sentry/android/okhttp/S…
2b0e79e 
…entryOkHttpInterceptorTest.kt

Co-authored-by: Alexander Dinauer <adinauer@users.noreply.github.com>
@lbloder
CR - create override for fromSentryTrace, add more complex pre-existi…
a46de63 
…ng headers, remove unused parameter in test
@adinauer adinauer mentioned this pull request Sep 13, 2022
Closed
5 tasks
@lbloder lbloder changed the title initial implementation for server-side baggage support and third part… Implementation for server-side baggage support and third party header handling Sep 15, 2022
@lbloder lbloder marked this pull request as ready for review September 16, 2022 08:30
@github-actions
Copy link
Contributor

github-actions bot commented Sep 19, 2022 •
edited

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 334.65 ms 358.82 ms 24.16 ms
Size 1.74 MiB 2.33 MiB 605.50 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
3d89dea 322.38 ms 350.82 ms 28.45 ms
1e4690d 354.69 ms 387.88 ms 33.19 ms
3d89dea 345.59 ms 364.06 ms 18.47 ms

App size

Revision Plain With Sentry Diff
3d89dea 1.74 MiB 2.33 MiB 604.92 KiB
1e4690d 1.74 MiB 2.33 MiB 604.92 KiB
3d89dea 1.74 MiB 2.33 MiB 604.92 KiB

Previous results on branch: feat/server_side_baggage

Startup times

Revision Plain With Sentry Diff
d0f05d5 355.79 ms 394.62 ms 38.83 ms

App size

Revision Plain With Sentry Diff
d0f05d5 1.74 MiB 2.33 MiB 605.50 KiB

@adinauer
Copy link
Member

LGTM!

@marandaneto do you want to give this a glance since you've seen multiple DS implementations and may notice if something's different here.

marandaneto
marandaneto reviewed Sep 20, 2022
View reviewed changes
sentry/src/main/java/io/sentry/TraceContext.java Outdated
@@ -389,4 +316,34 @@ private Exception missingRequiredFieldException(String field, ILogger logger) {
return exception;
}
}

@Override
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is needed for testing, I'd rather check the equality field per field, adding such methods increase the bundle size, and its not needed at runtime.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 22, 2022 •
edited

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against e4e169b

@adinauer
Copy link
Member

Looks like this can be merged now. @marandaneto should I go ahead an merge?

marandaneto
marandaneto approved these changes Sep 23, 2022
View reviewed changes
Copy link
Contributor

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

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

Thanks @lbloder

@adinauer adinauer merged commit 725a1ed into main Sep 23, 2022
@adinauer adinauer deleted the feat/server_side_baggage branch September 23, 2022 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bruno-garcia bruno-garcia bruno-garcia left review comments

@adinauer adinauer adinauer approved these changes

@marandaneto marandaneto marandaneto approved these changes

@romtsn romtsn Awaiting requested review from romtsn romtsn is a code owner

@stefanosiano stefanosiano Awaiting requested review from stefanosiano stefanosiano is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Server side baggage support
6 participants
@lbloder @codecov-commenter @adinauer @bruno-garcia @marandaneto @getsentry-bot