Keep baggage sample rate/rand as doubles by markushi · Pull Request #4279 · getsentry/sentry-java

markushi · 2025-03-21T18:46:03Z

📜 Description

Keeps the rate / rand values as doubles, until being transferred to TraceContext.

💡 Motivation and Context

We seem to be hitting some performance regression with 8.x.x where formatting the sample rate/rand seems to have some impact. I don't trust the profiled app fully, be there seems to be something to it:

Before:

After:

TODO: Check why this even being called 3 times (assuming it's for every scope right now)

💚 How did you test it?

📝 Checklist

I added tests to verify the changes.
No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
I updated the docs if needed.
I updated the wizard if needed.
Review from the native team if needed.
No breaking change or entry added to the changelog.
No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

github-actions · 2025-03-21T18:58:23Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	409.76 ms	484.30 ms	74.54 ms
Size	1.58 MiB	2.22 MiB	652.77 KiB

Previous results on branch: markushi/feat/retain-baggage-sample-rate-rand-as-double

Startup times

Revision	Plain	With Sentry	Diff
`2bb10ef`	439.64 ms	446.10 ms	6.46 ms
`e9d765b`	375.22 ms	445.06 ms	69.84 ms
`f2b6704`	406.86 ms	435.69 ms	28.83 ms
`902043e`	409.27 ms	486.46 ms	77.19 ms
`524a008`	464.70 ms	594.53 ms	129.84 ms

App size

Revision	Plain	With Sentry	Diff
`2bb10ef`	1.58 MiB	2.22 MiB	652.83 KiB
`e9d765b`	1.58 MiB	2.22 MiB	652.62 KiB
`f2b6704`	1.58 MiB	2.22 MiB	652.55 KiB
`902043e`	1.58 MiB	2.22 MiB	652.54 KiB
`524a008`	1.58 MiB	2.22 MiB	652.84 KiB

markushi · 2025-03-25T09:56:19Z

sentry/src/main/java/io/sentry/Baggage.java

@@ -231,8 +261,18 @@ public String getThirdPartyHeader() {
    }

    final Set<String> keys = new TreeSet<>(keyValues.keySet());


I know this is quite hacky, an alternative would be to let keyValues be a Map<String, String|Number> aka Map<String, Object>

We can revisit when more fields need to be added in the future and this current approach gets harder to maintain. I think it's fine for now.

markushi · 2025-03-25T10:28:37Z

sentry/src/main/java/io/sentry/Scope.java


  @ApiStatus.Internal
  @Override
  public @NotNull PropagationContext withPropagationContext(


What's the use case of returning a clone of PropagationContext here? As far as I can see we at least internally never seem to use the return type.

To avoid modifications from outside the withPropagationContext block, i.e. misusing this as a getter for modifying the instance.

adinauer · 2025-03-26T05:51:58Z

sentry/src/main/java/io/sentry/Baggage.java

@@ -231,8 +261,18 @@ public String getThirdPartyHeader() {
    }

    final Set<String> keys = new TreeSet<>(keyValues.keySet());


We can revisit when more fields need to be added in the future and this current approach gets harder to maintain. I think it's fine for now.

sentry/src/main/java/io/sentry/Baggage.java

adinauer · 2025-03-26T06:01:39Z

sentry/src/main/java/io/sentry/Scope.java


  @ApiStatus.Internal
  @Override
  public @NotNull PropagationContext withPropagationContext(


To avoid modifications from outside the withPropagationContext block, i.e. misusing this as a getter for modifying the instance.

adinauer · 2025-03-26T08:04:01Z

sentry/src/main/java/io/sentry/Scope.java

  private @NotNull List<Attachment> attachments = new CopyOnWriteArrayList<>();

-  private @NotNull PropagationContext propagationContext;
+  private @NotNull LazyEvaluator<PropagationContext> propagationContext;


Did we do any performance comparison as to whether this specific part actually improves anything?

No I didn't, and after sleeping over it I think it's better to revert this specific change: it makes everything less predictable 😅 I'll do a quick test, but I suspect the performance gains to be negligible.

Ok I did some testing using https://github.com/melix/jmh-gradle-plugin/, it's all within the same ballpark.
This is throughput, so higher values are better.

# Without Lazy Benchmark Mode Cnt Score Error Units ScopeBenchmark.ctor thrpt 4 4063093,683 ± 185929,301 ops/s ScopeBenchmark.getPropagationContext thrpt 4 4283007,711 ± 1670048,430 ops/s # With Lazy Benchmark Mode Cnt Score Error Units ScopeBenchmark.ctor thrpt 4 4639302,930 ± 518388,199 ops/s ScopeBenchmark.getPropagationContext thrpt 4 3769735,655 ± 102168,937 ops/s

Which ctor did you test there? The Scope(final @NotNull Scope scope) one or the Scope(final @NotNull SentryOptions options) one?

Scope(final @NotNull SentryOptions options) should only be used on init

Scope(final @NotNull Scope scope) is used by clone very often

In general I'd say, if this has little to no benefit, let's rather keep it simple and avoid LazyEvaluator here.

I did test Scope(final @NotNull SentryOptions options) so the one being used for init. Here's the code I used for testing. But yeah let's keep it without lazy, the ctor gain is just too small.

Co-authored-by: Alexander Dinauer <adinauer@users.noreply.github.com>

This reverts commit 398c5df.

…d-as-double

Keep baggage sample rate/rand as doubles

efe2dd9

markushi added 3 commits March 24, 2025 14:26

Fix tests

bc19aa7

Update Changelog

248dfed

Fix test

2fea4fb

markushi commented Mar 25, 2025

View reviewed changes

Lazily generate PropagationContext

398c5df

markushi commented Mar 25, 2025

View reviewed changes

markushi marked this pull request as ready for review March 25, 2025 10:28

markushi requested review from adinauer, lcian, romtsn and stefanosiano as code owners March 25, 2025 10:28

adinauer reviewed Mar 26, 2025

View reviewed changes

markushi and others added 2 commits March 26, 2025 09:21

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

188f500

Co-authored-by: Alexander Dinauer <adinauer@users.noreply.github.com>

Revert "Lazily generate PropagationContext"

d3775d4

This reverts commit 398c5df.

adinauer approved these changes Mar 26, 2025

View reviewed changes

adinauer mentioned this pull request Mar 26, 2025

New Performance API Wishlist #4215

Open

markushi enabled auto-merge (squash) March 26, 2025 11:34

Merge branch 'main' into markushi/feat/retain-baggage-sample-rate-ran…

014e7fb

…d-as-double

markushi merged commit 1090788 into main Mar 26, 2025
33 checks passed

markushi deleted the markushi/feat/retain-baggage-sample-rate-rand-as-double branch March 26, 2025 14:49

		@@ -231,8 +261,18 @@ public String getThirdPartyHeader() {
		}

		final Set<String> keys = new TreeSet<>(keyValues.keySet());

Uh oh!

Comments

Conversation

markushi commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

Uh oh!

github-actions bot commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Performance metrics 🚀

Previous results on branch: markushi/feat/retain-baggage-sample-rate-rand-as-double

Startup times

App size

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markushi Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

markushi commented Mar 21, 2025 •

edited

Loading

github-actions bot commented Mar 21, 2025 •

edited

Loading

markushi Mar 26, 2025 •

edited

Loading