Feature: Android profiling traces by stefanosiano · Pull Request #1897 · getsentry/sentry-java

stefanosiano · 2022-02-03T10:10:36Z

📜 Description

adds Android profiling whenever a transaction starts
adds a new envelope generation method to send profiling data
Bind traces to transactions
Ignore profiling for other transactions when they run at the same time

💡 Motivation and Context

We are integrating the Specto features in the Sentry sdk. This pr adds support for profiling Android methods whenever a sentry transaction is started

📝 Checklist

I reviewed the submitted code
I added tests to verify the changes
I updated the docs if needed
No breaking changes

🔮 Next steps

Add tests (will be added in another pr)

added a transaction listener (noOp on java sdk) that starts/stops android profiling when a transaction starts or is finished added an envelope generation method from android trace file added following SentryOptions: profilingTracesDirPath, profilingEnabled, maxTraceFileSize, profilingTracesIntervalMs, transactionListener

AndroidTraceTransactionListener now takes Hub in the constructor (for testability)

…e schedule() method ITransactionListener.onTransactionFinish() now returns a ProfilingTraceData, used to add the item to the transaction's envelope added io.sentry.traces.profiling-enable to manifest options profiling is now enabled by default

moved profilingTracesDirPath and profilingTracesIntervalMillis to SentryAndroidOptions updated ProfilingTraceData to be the payload of the profiling envelope item

romtsn

Some minor things, but LGTM 🚀

The tests are still missing, are you going to add them? Also, the PR is targeting main, I think I'd rather target some integration branch for now, if you want to add more stuff (like tests) in other PRs.

romtsn · 2022-02-23T13:27:28Z

+ private final transient @NotNull File traceFile;
+
+ // Device metadata
+ private final int android_api_level;


hm, I feel like checkstyle/spotless would yell here, because we should use camelCase

I ran it, but it didn't care at all. If we get rid of Gson then i can create a custom method that creates a json and use camelCase names for the variables

We got rid of Gson on Java v6 https://github.com/getsentry/sentry-java/tree/6.x.x

stefanosiano · 2022-03-07T10:33:25Z

For the serialization, should i rebase this branch to the 6.x.x branch and adapt to the ser/deser used there?

codecov-commenter · 2022-03-07T10:44:06Z

Codecov Report

Merging #1897 (7830a25) into feat/profiling/android (4afe30c) will decrease coverage by 0.75%.
The diff coverage is 36.36%.

@@ Coverage Diff @@## feat/profiling/android #1897 +/- ##
============================================================
- Coverage 75.46% 74.70% -0.76% - Complexity 2248 2260 +12 
============================================================
Files 225 228 +3 Lines 8036 8145 +109 Branches 851 867 +16 ============================================================
+ Hits 6064 6085 +21 - Misses 1562 1646 +84 - Partials 410 414 +4

Impacted Files	Coverage Δ
sentry/src/main/java/io/sentry/HubAdapter.java	`4.91% <0.00%> (-0.09%)`	⬇️
sentry/src/main/java/io/sentry/NoOpHub.java	`46.34% <ø> (ø)`
...ntry/src/main/java/io/sentry/NoOpSentryClient.java	`60.00% <ø> (ø)`
...main/java/io/sentry/NoOpSentryExecutorService.java	`40.00% <0.00%> (-10.00%)`	⬇️
...ry/src/main/java/io/sentry/ProfilingTraceData.java	`0.00% <0.00%> (ø)`
...n/java/io/sentry/transport/ReusableCountLatch.java	`88.23% <ø> (ø)`
sentry/src/main/java/io/sentry/util/FileUtils.java	`0.00% <0.00%> (ø)`
sentry/src/main/java/io/sentry/SentryClient.java	`84.63% <22.22%> (-1.35%)`	⬇️
sentry/src/main/java/io/sentry/Hub.java	`74.25% <50.00%> (-0.48%)`	⬇️
...c/main/java/io/sentry/NoOpTransactionProfiler.java	`50.00% <50.00%> (ø)`
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4afe30c...7830a25. Read the comment docs.

marandaneto · 2022-03-07T12:02:14Z

For the serialization, should i rebase this branch to the 6.x.x branch and adapt to the ser/deser used there?

I'd rather merge to the feature branch you already have.
Work on tests, etc... so we guarantee that nothing else from the SDK is broken.

bruno-garcia · 2022-03-08T18:54:47Z

For the serialization, should i rebase this branch to the 6.x.x branch and adapt to the ser/deser used there?
I'd rather merge to the feature branch you already have. Work on tests, etc... so we guarantee that nothing else from the SDK is broken.

+1 on merging first. Wrt 6.0 perhaps we need a bigger discussion and commitment from folks and set a timeline (making sure it doesn't negatively affect tracing's goals).

marandaneto · 2022-03-08T21:22:40Z

+ return null;
+ }
+
+ // The payload of the profiling item is a json that includes the trace file encoded with base64


Usually, base64 is 1/3 bigger, is there a reason to do so? Envelopes are able to send binary format.

We agreed with @phacops, our backend guy, that will expect a base64 string.
Also, the payload of the profile envelope is a json, so a string is a natural fit

I see, since it's a file, I was expecting to be an attachment that could be sent in bytes.
Reading the code now I understand, the envelope type is profile, which is a JSON of the ProfilingTraceData class, that contains a stacktrace field, and this field has the profiling file content in base64.

…, instead of deleting the whole dir and recreating it in the main thread AndroidTransactionProfiler now takes a buildInfoProvider in ctor profile envelope item now uses a CachedItem to read the trace file in the background

-device_is_emulator -device_cpu_frequencies -device_physical_memory_bytes -duration_ns added context to AndroidTransactionProfiler instead of packageInfo to retrieve memory info added isEmulator() to BuildInfoProvider

stefanosiano · 2022-03-10T21:15:16Z

I had to add few fields for the profiling envelope, so i made a few changes, but these should be the last one!

-stacktrace -> sampled_profile -stacktrace_id -> profile_id

bruno-garcia · 2022-03-14T17:03:42Z

🥳

github-actions · 2022-03-14T17:23:19Z

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

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- Android profiling traces ([#1897](https://github.com/getsentry/sentry-java/pull/1897))

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

Generated by 🚫 dangerJS against 7d08aca

* Added a transaction profiler (noOp on java sdk) that starts/stops android profiling when a transaction starts or is finished * Added a new envelope item type "profile", generated using android trace file. Its payload is a json represented by ProfilingTraceData * SentryExecutorService now uses a ScheduledExecutorService to allow the schedule() method * Added "profilingTracesDirPath" and "profilingTracesIntervalMillis" to SentryAndroidOptions * Added "profilingEnabled", "maxTraceFileSize" and "transactionProfiler" to SentryOptions * Added io.sentry.traces.profiling.enable to manifest options * Profiling is disabled by default * Profiling traces directory's content is deleted in the background when the options are initialized

stefanosiano requested review from maciejwalkowiak, marandaneto and romtsn as code owners February 3, 2022 10:10

stefanosiano marked this pull request as draft February 3, 2022 10:11

stefanosiano requested a review from bruno-garcia February 3, 2022 10:13