`isTraceSampling` is now on by default. `tracingOrigins` has been replaced by `tracePropagationTargets` #2255

lbloder · 2022-09-23T08:19:38Z

📜 Description

Deprecates tracingOrigins and introduces tracePropagationTargets as a replacement
Also deprecates the traceSampling flag

💡 Motivation and Context

Fixes #2243, fixes #2244, fixes #2205

💚 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

…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

…gets

# 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

github-actions · 2022-09-23T08:20:59Z

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

Generated by 🚫 dangerJS against da0ccc3

github-actions · 2022-09-23T08:35:50Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	297.84 ms	334.54 ms	36.70 ms
Size	1.73 MiB	2.29 MiB	578.70 KiB

Baseline results on branch: main

Startup times

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

Changelog is missing. It should mention that isTraceSampling is now on by default. Left some more comments on the default handling.

sentry-android-core/src/test/java/io/sentry/android/core/ManifestMetadataReaderTest.kt

sentry/src/main/java/io/sentry/ExternalOptions.java

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

codecov-commenter · 2022-09-28T08:10:53Z

Codecov Report

Base: 80.11% // Head: 80.11% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (e9bde17) compared to base (c5ccd8a).
Patch coverage: 76.47% of modified lines in pull request are covered.

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

Impacted Files	Coverage Δ
.../io/sentry/apollo3/SentryApollo3HttpInterceptor.kt	`79.77% <0.00%> (-1.13%)`	⬇️
...racing/SentrySpanClientHttpRequestInterceptor.java	`0.00% <0.00%> (ø)`
...ring/tracing/SentrySpanClientWebRequestFilter.java	`72.72% <0.00%> (+9.09%)`	⬆️
sentry/src/main/java/io/sentry/SentryOptions.java	`82.33% <76.92%> (-0.74%)`	⬇️
...entry/src/main/java/io/sentry/ExternalOptions.java	`97.84% <84.21%> (-2.16%)`	⬇️
...in/java/io/sentry/openfeign/SentryFeignClient.java	`95.58% <100.00%> (-1.48%)`	⬇️
sentry/src/main/java/io/sentry/Dsn.java	`94.28% <100.00%> (+0.16%)`	⬆️
sentry/src/main/java/io/sentry/TracingOrigins.java	`77.77% <100.00%> (ø)`
... and 4 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.

sentry/src/main/java/io/sentry/Dsn.java

Co-authored-by: Roman Zavarnitsyn <rom4ek93@gmail.com>

sentry/src/main/java/io/sentry/TracingOrigins.java

romtsn · 2022-09-28T09:03:20Z

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

+  @Deprecated
+  @SuppressWarnings("InlineMeSuggester")
+  @ApiStatus.Internal
+  public void setTracingOrigins(List<String> tracingOrigins) {


Do we even need this one, if it wasn't here before?

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.

I'd not add a new public API as @Deprecated, I'd rather fix the issue if there's one using the new origins.

Not sure how this broke. Can investigate for a bit but would prefer not to delay things because of it.

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.

romtsn · 2022-09-28T09:03:52Z

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

+  }
+
+  @ApiStatus.Internal
+  public void setTracePropagationTargets(List<String> tracePropagationTargets) {


Suggested change

public void setTracePropagationTargets(List<String> tracePropagationTargets) {

public void setTracePropagationTargets(final List<String> tracePropagationTargets) {

also, can it be non-nullable?

Might make sense to allow passing null to restore the default regex.

sentry/src/main/java/io/sentry/Dsn.java

sentry/src/main/java/io/sentry/ExternalOptions.java

marandaneto · 2022-09-28T09:28:05Z

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

+  private @Nullable List<String> tracePropagationTargets = null;
+
+  private final @NotNull List<String> defaultTracePropagationTargets =
+      Collections.singletonList(".*");


Can we have just a list with the default value?

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.

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.

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).

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

marandaneto · 2022-09-28T10:13:06Z

Missing general finals and annotations for Nullable for Non Nullables.
I thought that we had a static code analyzer for the annotations already, don't we?

…tions; CR

lbloder and others added 18 commits August 30, 2022 01:16

initial implementation for server-side baggage support and third part…

0205111

…y header handling in integrations

Format code

b2db551

add changelog, dump api

8d2ef82

Merge branch 'feat/server_side_baggage' of github.com:getsentry/sentr…

2fbb9fe

…y-java into feat/server_side_baggage

fix tests

a06ad71

add baggage merge tests

a383022

Merge branch 'main' into feat/server_side_baggage

b34bc54

# Conflicts: # CHANGELOG.md

add baggage merge test for Spring RestTemplate

f329bf7

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>

CR - create override for fromSentryTrace, add more complex pre-existi…

a46de63

…ng headers, remove unused parameter in test

Merge branch 'main' into feat/server_side_baggage

6d03e10

re-introduce max-member limit

f42fbb8

deprecate traceSampling flag, deprecate tracingOrigins in favor of ne…

b925630

…wly introduced tracePropagationOrigins

Merge branch 'main' into feat/server_side_baggage

a79c650

Merge branch 'feat/server_side_baggage' into feat/tracePropagationTar…

fc52172

…gets

remove equal and hashcode from TraceContext, adapt tests

b0b0a9f

Merge branch 'feat/server_side_baggage' into feat/tracePropagationTar…

9580dc1

…gets

adinauer requested changes Sep 23, 2022

View reviewed changes

sentry-android-core/src/test/java/io/sentry/android/core/ManifestMetadataReaderTest.kt Show resolved Hide resolved

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

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

lbloder and others added 4 commits September 27, 2022 20:55

[WIP] default value for tracePropagationTargets

27b59a4

Format code

c726c5d

Allow disabling via tracePropagationTargets; fix tests; add tests

070949a

Merge branch 'main' into feat/tracePropagationTargets

0d6ccc6

adinauer marked this pull request as ready for review September 28, 2022 07:51

adinauer requested review from romtsn and stefanosiano as code owners September 28, 2022 07:51

romtsn reviewed Sep 28, 2022

View reviewed changes

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

adinauer and others added 2 commits September 28, 2022 10:50

Add more tests; fix spring parsing for tracing-origins

3d018bc

Update sentry/src/main/java/io/sentry/Dsn.java

3c4da43

Co-authored-by: Roman Zavarnitsyn <rom4ek93@gmail.com>

adinauer changed the title ~~Replace tracingOrigins with tracePropagationTargets~~ isTraceSampling is now on by default. tracingOrigins has been replaced by tracePropagationTargets Sep 28, 2022

romtsn reviewed Sep 28, 2022

View reviewed changes

sentry/src/main/java/io/sentry/TracingOrigins.java Show resolved Hide resolved

getsentry-bot and others added 2 commits September 28, 2022 08:57

Format code

7c1ce3f

Add changelog; fix build

e9bde17

romtsn reviewed Sep 28, 2022

View reviewed changes

romtsn approved these changes Sep 28, 2022

View reviewed changes

marandaneto reviewed Sep 28, 2022

View reviewed changes

sentry/src/main/java/io/sentry/Dsn.java Show resolved Hide resolved

marandaneto reviewed Sep 28, 2022

View reviewed changes

sentry/src/main/java/io/sentry/ExternalOptions.java Show resolved Hide resolved

marandaneto reviewed Sep 28, 2022

View reviewed changes

sentry/src/main/java/io/sentry/SentryOptions.java Show resolved Hide resolved

marandaneto reviewed Sep 28, 2022

View reviewed changes

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

marandaneto reviewed Sep 28, 2022

View reviewed changes

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

adinauer added 2 commits September 28, 2022 17:39

Rename TracingOrigins; remove addTracePropagationTarget from SentryOp…

5955807

…tions; CR

CR

a3c9a36

adinauer requested a review from marandaneto September 28, 2022 16:24

adinauer and others added 2 commits September 29, 2022 13:18

Fix JavaDoc

0420714

Merge branch 'main' into feat/tracePropagationTargets

46d0849

adinauer approved these changes Sep 29, 2022

View reviewed changes

Fix test after merging in main

da0ccc3

adinauer merged commit 02325ad into main Sep 29, 2022

adinauer deleted the feat/tracePropagationTargets branch September 29, 2022 13:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`isTraceSampling` is now on by default. `tracingOrigins` has been replaced by `tracePropagationTargets` #2255

`isTraceSampling` is now on by default. `tracingOrigins` has been replaced by `tracePropagationTargets` #2255

lbloder commented Sep 23, 2022 •

edited by adinauer

github-actions bot commented Sep 23, 2022 •

edited

github-actions bot commented Sep 23, 2022 •

edited

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/tracePropagationTargets

Startup times

App size

adinauer left a comment

codecov-commenter commented Sep 28, 2022 •

edited

romtsn Sep 28, 2022

adinauer Sep 28, 2022

marandaneto Sep 28, 2022

adinauer Sep 28, 2022

adinauer Sep 28, 2022

romtsn Sep 28, 2022 •

edited

adinauer Sep 28, 2022

marandaneto Sep 28, 2022

adinauer Sep 28, 2022

marandaneto Sep 28, 2022

adinauer Sep 28, 2022

marandaneto commented Sep 28, 2022

	public void setTracePropagationTargets(List<String> tracePropagationTargets) {
	public void setTracePropagationTargets(final List<String> tracePropagationTargets) {

isTraceSampling is now on by default. tracingOrigins has been replaced by tracePropagationTargets #2255

isTraceSampling is now on by default. tracingOrigins has been replaced by tracePropagationTargets #2255

Conversation

lbloder commented Sep 23, 2022 • edited by adinauer

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Sep 23, 2022 • edited

github-actions bot commented Sep 23, 2022 • edited

Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/tracePropagationTargets

Startup times

App size

adinauer left a comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 28, 2022 • edited

Codecov Report

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

Choose a reason for hiding this comment

romtsn Sep 28, 2022 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marandaneto commented Sep 28, 2022

`isTraceSampling` is now on by default. `tracingOrigins` has been replaced by `tracePropagationTargets` #2255

`isTraceSampling` is now on by default. `tracingOrigins` has been replaced by `tracePropagationTargets` #2255

lbloder commented Sep 23, 2022 •

edited by adinauer

github-actions bot commented Sep 23, 2022 •

edited

github-actions bot commented Sep 23, 2022 •

edited

codecov-commenter commented Sep 28, 2022 •

edited

romtsn Sep 28, 2022 •

edited