Profiling - Deduplication and cleanup #4681

lbloder · 2025-08-29T09:59:14Z

📜 Description

Follow-up of: #4576

Fixes comments from previous PRs
Fixes method timing sent to sentry by not aggregating JFR events
Fixes RateLimiter Handling for profile_chunk and profile_chunk_ui

💡 Motivation and Context

💚 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

Add option to filter threads in converter to reduce payload and data that needs to be handled in the backend

…with cross platform frameworks

…plication work

…atelimiting

…nkUi drops

github-actions · 2025-08-29T09:59:35Z

🚨 Detected changes in high risk code 🚨

High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:

sentry/src/main/java/io/sentry/transport/RateLimiter.java

github-actions · 2025-08-29T09:59:40Z

	Fails
🚫	Please consider adding a changelog entry for the next release.

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

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- Profiling - Deduplication and cleanup ([#4681](https://github.com/getsentry/sentry-java/pull/4681))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description or adding a skip-changelog label.

Generated by 🚫 dangerJS against 524a32a

github-actions · 2025-08-29T10:01:46Z

🚨 Detected changes in high risk code 🚨

High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:

sentry/src/main/java/io/sentry/transport/RateLimiter.java

github-actions · 2025-08-29T10:14:40Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	415.48 ms	485.28 ms	69.80 ms
Size	1.58 MiB	2.09 MiB	521.84 KiB

Previous results on branch: feat/continuous-profiling-03

Startup times

Revision	Plain	With Sentry	Diff
`d6f8356`	392.08 ms	454.42 ms	62.34 ms
`066d89d`	377.51 ms	434.20 ms	56.69 ms
`7dcbbbd`	459.22 ms	509.85 ms	50.62 ms

App size

Revision	Plain	With Sentry	Diff
`d6f8356`	1.58 MiB	2.09 MiB	521.68 KiB
`066d89d`	1.58 MiB	2.09 MiB	521.85 KiB
`7dcbbbd`	1.58 MiB	2.09 MiB	521.68 KiB

…deduplicate stacks

…entry-java into feat/continuous-profiling-03

github-actions · 2025-09-05T08:43:04Z

🚨 Detected changes in high risk code 🚨

High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:

sentry/src/main/java/io/sentry/transport/RateLimiter.java

github-actions · 2025-09-08T06:35:05Z

🚨 Detected changes in high risk code 🚨

High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:

sentry/src/main/java/io/sentry/transport/RateLimiter.java

lbloder · 2025-09-08T07:08:33Z

@sentry review

lbloder · 2025-09-08T07:08:41Z

cursor review

cursor

✅ Bugbot reviewed your changes and found no bugs!

seer-by-sentry · 2025-09-08T07:16:50Z

sentry/src/main/java/io/sentry/protocol/SentryStackFrame.java

    public static final String POST_CONTEXT = "post_context";
  }

+  @Override
+  public boolean equals(Object o) {
+    if (o == null || getClass() != o.getClass()) return false;
+    SentryStackFrame that = (SentryStackFrame) o;
+    return Objects.equals(preContext, that.preContext)
+        && Objects.equals(postContext, that.postContext)
+        && Objects.equals(vars, that.vars)
+        && Objects.equals(framesOmitted, that.framesOmitted)
+        && Objects.equals(filename, that.filename)
+        && Objects.equals(function, that.function)
+        && Objects.equals(module, that.module)
+        && Objects.equals(lineno, that.lineno)
+        && Objects.equals(colno, that.colno)
+        && Objects.equals(absPath, that.absPath)
+        && Objects.equals(contextLine, that.contextLine)
+        && Objects.equals(inApp, that.inApp)
+        && Objects.equals(_package, that._package)
+        && Objects.equals(_native, that._native)
+        && Objects.equals(platform, that.platform)
+        && Objects.equals(imageAddr, that.imageAddr)
+        && Objects.equals(symbolAddr, that.symbolAddr)
+        && Objects.equals(instructionAddr, that.instructionAddr)
+        && Objects.equals(addrMode, that.addrMode)
+        && Objects.equals(symbol, that.symbol)
+        && Objects.equals(unknown, that.unknown)
+        && Objects.equals(rawFunction, that.rawFunction)
+        && Objects.equals(lock, that.lock);
+  }
+
+  @Override
+  public int hashCode() {
+    return Objects.hash(
+        preContext,
+        postContext,
+        vars,
+        framesOmitted,
+        filename,
+        function,
+        module,
+        lineno,
+        colno,
+        absPath,
+        contextLine,
+        inApp,
+        _package,
+        _native,
+        platform,
+        imageAddr,
+        symbolAddr,
+        instructionAddr,
+        addrMode,
+        symbol,
+        unknown,
+        rawFunction,
+        lock);
+  }


The vars field is documented as Map<String, String> in the class comment but the actual field type has changed to Map<String, Object> according to the repository context. This mismatch could lead to confusion. The equals and hashCode methods should exclude mutable fields that can change during the object's lifetime, but here vars is included which may cause issues if the map is modified after the object's hash is computed.

_{Did we get this right? 👍 / 👎 to inform future reviews.}

seer-by-sentry · 2025-09-08T07:16:55Z

.../src/main/java/io/sentry/asyncprofiler/convert/JfrAsyncProfilerToSentryProfileConverter.java

 import java.util.List;
+import java.util.Map;


The frame deduplication using HashMap with SentryStackFrame as key relies on the newly added equals/hashCode methods. However, if any of the frame fields are mutable and change after being used as a key, this could lead to inconsistent behavior. Consider making the frame objects immutable after creation or using a different deduplication strategy.

_{Did we get this right? 👍 / 👎 to inform future reviews.}

github-actions · 2025-09-08T08:06:48Z

🚨 Detected changes in high risk code 🚨

High-risk code has higher potential to break the SDK and may be hard to test. To prevent severe bugs, apply the rollout process for releasing such changes and be extra careful when changing and reviewing these files:

sentry/src/main/java/io/sentry/transport/RateLimiter.java

adinauer

WIP

adinauer · 2025-09-12T10:41:04Z

.../src/main/java/io/sentry/asyncprofiler/convert/JfrAsyncProfilerToSentryProfileConverter.java

      }
    }

+    private long resolveThreadId(int eventThreadId) {
+      return jfr.threads.get(eventThreadId) != null


l could extract a var here

m what does it mean if it can't be found in javaThreads? Does it mean it's simply not remapped from the original ID? Could we be reporting garbage data by using the original ID directly if remapping data is missing? e.g. eventThreadId = 9, we can't find it in javaThreads but the real thread ID was actually 117

Yeah, potentially, if the orig. threadId somehow matches a different javaThreadId then we might map it to the wrong thread.
We could set it to a default thread id, like -1 if we can't resolve it. Or drop the event. WDYT?

adinauer · 2025-09-15T12:14:16Z

.../src/main/java/io/sentry/asyncprofiler/convert/JfrAsyncProfilerToSentryProfileConverter.java

    }

    @Override
-    public void visit(Event event, long value) {
+    public void visit(Event event, long samples, long value) {


h any exception here, e.g. when processing a single stack frame would cause the whole chunk to be thrown away if I understand correctly. Is that what we want? Can we only drop broken frames instead?

How detailed do you think we should do that? just drop the whole event? i.e. catch any exception inside the visit method and just keep going?

adinauer · 2025-09-15T12:22:14Z

sentry-async-profiler/api/sentry-async-profiler.api

@@ -8,6 +8,15 @@ public final class io/sentry/asyncprofiler/convert/JfrAsyncProfilerToSentryProfi
 	public static fun convertFromFileStatic (Ljava/nio/file/Path;)Lio/sentry/protocol/profiling/SentryProfile;
 }

+public final class io/sentry/asyncprofiler/convert/NonAggregatingEventCollector : io/sentry/asyncprofiler/vendor/asyncprofiler/jfr/event/EventCollector {


Not related to this code.

l do we need EventCollector?

adinauer · 2025-09-15T12:26:12Z

.../src/main/java/io/sentry/asyncprofiler/convert/JfrAsyncProfilerToSentryProfileConverter.java

          continue;
        }

        SentryStackFrame frame = createStackFrame(element);
-        sentryProfile.getFrames().add(frame);
+        frame.setNative(isNativeFrame(types[i]));


l looks like this will always be false

adinauer · 2025-09-16T08:10:41Z

...y-async-profiler/src/main/java/io/sentry/asyncprofiler/profiling/JavaContinuousProfiler.java

+
+    File profileDir = new File(profilingTracesDirPath);
+
+    if (!profileDir.canWrite() || !profileDir.exists()) {


l can we split this up to provide a message that more easily helps pinpoint the problem?

adinauer · 2025-09-16T08:15:52Z

...y-async-profiler/src/main/java/io/sentry/asyncprofiler/profiling/JavaContinuousProfiler.java

  private final @NotNull AtomicBoolean isClosed = new AtomicBoolean(false);
  private @NotNull SentryDate startProfileChunkTimestamp = new SentryNanotimeDate();

  private @NotNull String filename = "";

-  private final @NotNull AsyncProfiler profiler;
+  private @Nullable AsyncProfiler profiler;


l an alternative to making this nullable could have been to handle the exception in AsyncProfilerContinuousProfilerProvider and simply return a NoOpContinuousProfiler. This would keep the code here simpler.

...y-async-profiler/src/main/java/io/sentry/asyncprofiler/profiling/JavaContinuousProfiler.java

adinauer · 2025-09-16T08:25:59Z

...y-async-profiler/src/main/java/io/sentry/asyncprofiler/profiling/JavaContinuousProfiler.java

@@ -101,7 +130,6 @@ private boolean init() {
    return true;
  }

-  @SuppressWarnings("ReferenceEquality")
  @Override
  public void startProfiler(


m should we catch here just in case something goes wrong?

adinauer · 2025-09-16T08:30:32Z

sentry-async-profiler/src/main/java/io/sentry/asyncprofiler/vendor/asyncprofiler/README.md

@@ -0,0 +1,4 @@
+# Vendored AsyncProfiler code for converting JFR Files
+- Vendored-in from commit fe1bc66d4b6181413847f6bbe5c0db805f3e9194 of repository: [email protected]:async-profiler/async-profiler.git


Suggested change

- Vendored-in from commit fe1bc66d4b6181413847f6bbe5c0db805f3e9194 of repository: git@github.com:async-profiler/async-profiler.git

- Vendored-in from commit https://github.com/async-profiler/async-profiler/tree/fe1bc66d4b6181413847f6bbe5c0db805f3e9194

adinauer · 2025-09-16T10:27:59Z

...rc/test/java/io/sentry/asyncprofiler/convert/JfrAsyncProfilerToSentryProfileConverterTest.kt

+      // No need to check exact decimal places as this depends on JFR precision
+      assertTrue(
+        timestamp < System.currentTimeMillis() / 1000.0 + 360,
+        "Timestamp should be recent",


I'm confused here. It can't be recent if we use a pre-existing JFR file.
Is the message wrong here?
Why do we need to add 360?
Is this intended for creating the file within the test and asserting it's at most 10s old? Then this test might fail on slow CI.

sentry/src/main/java/io/sentry/SentryEnvelopeItem.java

sentry/src/main/java/io/sentry/transport/RateLimiter.java

lbloder added 12 commits August 4, 2025 13:01

add readme and info about commit of the source repository

bb12990

delete jfr file on jvm exit

14d2f3b

further split into smaller methods

bdd7b3a

deduplicate frames in order to save bandwidth, add converter tests

1b88a3d

remove Platform Enum, use string constants instead for compatibility …

81aef38

…with cross platform frameworks

implement equals and hashcode for SentryStackFrame to make frame dedu…

8f32429

…plication work

bump api

b7645f2

improve error handling, fix start stop start flow

66ae530

add new testfile

037b237

calculate ticksPerNanosecond in constructor

2979b16

adapt Ratelimiter to check for both ProfileChunk and ProfileChunkUi r…

a32b71e

…atelimiting

update ratelimiter test to check for both profileChunk and profileChu…

4b1e57e

…nkUi drops

use string constant instead of string

8992018

Format code

c9a6bc0

lbloder added 3 commits September 4, 2025 17:41

add non aggregating event collector to send each event individually, …

451f191

…deduplicate stacks

adapt converter tests to new non-aggregated converter

885f0ab

Merge branch 'feat/continuous-profiling-03' of github.com:getsentry/s…

7ae393e

…entry-java into feat/continuous-profiling-03

Format code

b1701c5

lbloder mentioned this pull request Sep 8, 2025

Feat/poc continuous profiling #4556

Draft

9 tasks

add logging to loadProfileConverter

30cb14b

Format code

94a37b1

lbloder changed the title ~~add readme and info about commit of the source repository~~ Profiling - Deduplication and cleanup Sep 8, 2025

lbloder mentioned this pull request Sep 8, 2025

Support ProfileLifecycle.TRACE #4576

Open

10 tasks

cursor bot reviewed Sep 8, 2025

View reviewed changes

seer-by-sentry bot reviewed Sep 8, 2025

View reviewed changes

fix duplication of events

524a32a

adinauer reviewed Sep 15, 2025

View reviewed changes

adinauer reviewed Sep 16, 2025

View reviewed changes


		File profileDir = new File(profilingTracesDirPath);

		if (!profileDir.canWrite() \|\| !profileDir.exists()) {

		@@ -0,0 +1,4 @@
		# Vendored AsyncProfiler code for converting JFR Files
		- Vendored-in from commit fe1bc66d4b6181413847f6bbe5c0db805f3e9194 of repository: [email protected]:async-profiler/async-profiler.git

	- Vendored-in from commit fe1bc66d4b6181413847f6bbe5c0db805f3e9194 of repository: git@github.com:async-profiler/async-profiler.git
	- Vendored-in from commit https://github.com/async-profiler/async-profiler/tree/fe1bc66d4b6181413847f6bbe5c0db805f3e9194

Uh oh!

Profiling - Deduplication and cleanup #4681

Are you sure you want to change the base?

Profiling - Deduplication and cleanup #4681

Uh oh!

Conversation

lbloder commented Aug 29, 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 Aug 29, 2025

🚨 Detected changes in high risk code 🚨

Uh oh!

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

Uh oh!

Instructions and example for changelog

Uh oh!

github-actions bot commented Aug 29, 2025

🚨 Detected changes in high risk code 🚨

Uh oh!

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

Uh oh!

Performance metrics 🚀

Previous results on branch: feat/continuous-profiling-03

Startup times

App size

Uh oh!

github-actions bot commented Sep 5, 2025

🚨 Detected changes in high risk code 🚨

Uh oh!

github-actions bot commented Sep 8, 2025

🚨 Detected changes in high risk code 🚨

Uh oh!

lbloder commented Sep 8, 2025

Uh oh!

lbloder commented Sep 8, 2025

Uh oh!

cursor bot left a comment

Choose a reason for hiding this comment

✅ Bugbot reviewed your changes and found no bugs!

Uh oh!

seer-by-sentry bot Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

seer-by-sentry bot Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 8, 2025

🚨 Detected changes in high risk code 🚨

Uh oh!

adinauer left a comment

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!

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!

lbloder commented Aug 29, 2025 •

edited

Loading

github-actions bot commented Aug 29, 2025 •

edited

Loading

github-actions bot commented Aug 29, 2025 •

edited

Loading