Repository navigation
isTraceSampling is now on by default. tracingOrigins has been replaced by tracePropagationTargets - #2255
Conversation
…y header handling in integrations
…y-java into feat/server_side_baggage
# Conflicts: # CHANGELOG.md
…entryOkHttpInterceptorTest.kt Co-authored-by: Alexander Dinauer <adinauer@users.noreply.github.com>
…ng headers, remove unused parameter in test
…wly introduced tracePropagationOrigins
# Conflicts: # sentry-android-navigation/src/test/java/io/sentry/android/navigation/SentryNavigationListenerTest.kt # sentry-apollo-3/src/main/java/io/sentry/apollo3/SentryApollo3HttpInterceptor.kt # sentry-openfeign/src/test/kotlin/io/sentry/openfeign/SentryFeignClientTest.kt
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 2f079a1 | 296.91 ms | 337.43 ms | 40.51 ms |
| 3d89dea | 345.59 ms | 364.06 ms | 18.47 ms |
| 3d89dea | 322.38 ms | 350.82 ms | 28.45 ms |
| 1e4690d | 354.69 ms | 387.88 ms | 33.19 ms |
| c5ccd8a | 329.98 ms | 365.52 ms | 35.54 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 2f079a1 | 1.74 MiB | 2.33 MiB | 605.56 KiB |
| 3d89dea | 1.74 MiB | 2.33 MiB | 604.92 KiB |
| 3d89dea | 1.74 MiB | 2.33 MiB | 604.92 KiB |
| 1e4690d | 1.74 MiB | 2.33 MiB | 604.92 KiB |
| c5ccd8a | 1.74 MiB | 2.33 MiB | 607.44 KiB |
Previous results on branch: feat/tracePropagationTargets
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 92045be | 302.79 ms | 346.37 ms | 43.58 ms |
| 0ed7ccd | 302.50 ms | 316.12 ms | 13.62 ms |
| 34d00c2 | 282.30 ms | 355.46 ms | 73.16 ms |
| 8e1d7ef | 342.52 ms | 384.23 ms | 41.71 ms |
| 581da6d | 342.68 ms | 385.83 ms | 43.15 ms |
| 901599e | 293.43 ms | 369.43 ms | 76.00 ms |
| 2e202e6 | 316.35 ms | 339.57 ms | 23.22 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 92045be | 1.74 MiB | 2.33 MiB | 607.94 KiB |
| 0ed7ccd | 1.73 MiB | 2.29 MiB | 578.70 KiB |
| 34d00c2 | 1.74 MiB | 2.33 MiB | 607.87 KiB |
| 8e1d7ef | 1.74 MiB | 2.33 MiB | 607.93 KiB |
| 581da6d | 1.73 MiB | 2.29 MiB | 578.70 KiB |
| 901599e | 1.74 MiB | 2.33 MiB | 605.58 KiB |
| 2e202e6 | 1.74 MiB | 2.33 MiB | 608.00 KiB |
adinauer
left a comment
There was a problem hiding this comment.
Changelog is missing. It should mention that isTraceSampling is now on by default. Left some more comments on the default handling.
Codecov ReportBase: 80.11% // Head: 80.11% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #2255 +/- ##
=========================================
Coverage 80.11% 80.11%
- Complexity 3408 3418 +10
=========================================
Files 241 241
Lines 12601 12633 +32
Branches 1689 1697 +8
=========================================
+ Hits 10095 10121 +26
- Misses 1868 1871 +3
- Partials 638 641 +3
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. |
Co-authored-by: Roman Zavarnitsyn <rom4ek93@gmail.com>
isTraceSampling is now on by default. tracingOrigins has been replaced by tracePropagationTargets
| @Deprecated | ||
| @SuppressWarnings("InlineMeSuggester") | ||
| @ApiStatus.Internal | ||
| public void setTracingOrigins(List<String> tracingOrigins) { |
There was a problem hiding this comment.
Do we even need this one, if it wasn't here before?
There was a problem hiding this comment.
Without it the test for supporting the tracing-origins parsing for Spring Boot failed because it couldn't find the setter. Didn't investigate why that is. Seemed like the easiest way out to just add it and instantly deprecate. I could probably move it to SentryProperties or investigate if you prefer.
There was a problem hiding this comment.
I'd not add a new public API as @Deprecated, I'd rather fix the issue if there's one using the new origins.
There was a problem hiding this comment.
Not sure how this broke. Can investigate for a bit but would prefer not to delay things because of it.
There was a problem hiding this comment.
Ah just had a talk with @lbloder. Due to changing the return value of the getter to unmodifiable list Spring is no longer able to use the getter and modify the list directly.
To avoid the unmodifiable list we'd have to use a list where the default entry is in it. The probem then becomes how and when we remove the default entry. For Spring we don't have control over the behaviour as it is done automatically and there's no Sentry code like we have for Manifest and ExternalOptions where we can check whether any value was set and can then decide whether to use the default.
| } | ||
|
|
||
| @ApiStatus.Internal | ||
| public void setTracePropagationTargets(List<String> tracePropagationTargets) { |
There was a problem hiding this comment.
| public void setTracePropagationTargets(List<String> tracePropagationTargets) { | |
| public void setTracePropagationTargets(final List<String> tracePropagationTargets) { |
also, can it be non-nullable?
There was a problem hiding this comment.
Might make sense to allow passing null to restore the default regex.
| private @Nullable List<String> tracePropagationTargets = null; | ||
|
|
||
| private final @NotNull List<String> defaultTracePropagationTargets = | ||
| Collections.singletonList(".*"); |
There was a problem hiding this comment.
Can we have just a list with the default value?
There was a problem hiding this comment.
Then we'd need to do checks like "does the list only contain the default item" in the add method. Or we only allow setting a list but not adding individually. This would cause problems with continuing to support the deprecated tracingOrigins.
There was a problem hiding this comment.
We don't need to check if it contains already, if it's duplicated, it's a user mistake (that won't have a side effect), we only need to make it mutable, either adding new cases or removing what's in there.
There was a problem hiding this comment.
As discussed earlier we're keeping it this way. It works as expected and due to the now deprecated addTracingOrigin method we still need to make sure users who are calling it only end up with entries they put in there and not the default value (that sends everywhere).
|
Missing general |
📜 Description
Deprecates
tracingOriginsand introducestracePropagationTargetsas a replacementAlso deprecates the
traceSamplingflag💡 Motivation and Context
Fixes #2243, fixes #2244, fixes #2205
💚 How did you test it?
📝 Checklist
🔮 Next steps