Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/profiling/android #1973

Merged
merged 26 commits into from
Apr 6, 2022
Merged

Feat/profiling/android #1973

merged 26 commits into from
Apr 6, 2022

Conversation

stefanosiano
Copy link
Member

@stefanosiano stefanosiano commented Apr 1, 2022

📜 Description

Add profiling feature to Sentry v6

#skip-changelog

stefanosiano and others added 22 commits March 14, 2022 18:03
* 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
* 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
added ser/deser logic to ProfilingTraceData (+ tests)
…t/profiling/android

# Conflicts:
#	CHANGELOG.md
#	gradle.properties
#	sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransactionProfilerTest.kt
#	sentry-android-core/src/test/java/io/sentry/android/core/SentryAndroidOptionsTest.kt
#	sentry/api/sentry.api
#	sentry/src/main/java/io/sentry/ProfilingTraceData.java
#	sentry/src/main/java/io/sentry/SentryEnvelopeItem.java
#	sentry/src/test/java/io/sentry/HubTest.kt
#	sentry/src/test/java/io/sentry/SentryClientTest.kt
#	sentry/src/test/java/io/sentry/SentryEnvelopeItemTest.kt
#	sentry/src/test/java/io/sentry/SentryTracerTest.kt
* 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
added ser/deser logic to ProfilingTraceData (+ tests)
* 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
…t/profiling/android

# Conflicts:
#	CHANGELOG.md
#	gradle.properties
#	sentry-android-core/api/sentry-android-core.api
#	sentry-android-core/src/main/java/io/sentry/android/core/AndroidOptionsInitializer.java
#	sentry-android-core/src/main/java/io/sentry/android/core/IBuildInfoProvider.java
#	sentry-android-core/src/test/java/io/sentry/android/core/ActivityLifecycleIntegrationTest.kt
#	sentry-android-core/src/test/java/io/sentry/android/core/AndroidOptionsInitializerTest.kt
#	sentry-apollo/src/test/java/io/sentry/apollo/SentryApolloInterceptorTest.kt
#	sentry-spring/src/test/kotlin/io/sentry/spring/tracing/SentryTracingFilterTest.kt
#	sentry-spring/src/test/kotlin/io/sentry/spring/tracing/SentryTransactionAdviceTest.kt
#	sentry/api/sentry.api
#	sentry/src/main/java/io/sentry/Hub.java
#	sentry/src/main/java/io/sentry/HubAdapter.java
#	sentry/src/main/java/io/sentry/IHub.java
#	sentry/src/main/java/io/sentry/ISentryClient.java
#	sentry/src/main/java/io/sentry/NoOpHub.java
#	sentry/src/main/java/io/sentry/NoOpSentryClient.java
#	sentry/src/main/java/io/sentry/SentryClient.java
#	sentry/src/test/java/io/sentry/HubTest.kt
#	sentry/src/test/java/io/sentry/SentryClientTest.kt
#	sentry/src/test/java/io/sentry/SentryOptionsTest.kt
#	sentry/src/test/java/io/sentry/SentryTracerTest.kt
added ser/deser logic to ProfilingTraceData (+ tests)
several updates for v6
@codecov-commenter
Copy link

Codecov Report

Merging #1973 (5732ed1) into 6.x.x (4afe30c) will increase coverage by 5.11%.
The diff coverage is 88.59%.

@@             Coverage Diff              @@
##              6.x.x    #1973      +/-   ##
============================================
+ Coverage     75.46%   80.57%   +5.11%     
- Complexity     2248     3024     +776     
============================================
  Files           225      217       -8     
  Lines          8036    11215    +3179     
  Branches        851     1498     +647     
============================================
+ Hits           6064     9036    +2972     
- Misses         1562     1620      +58     
- Partials        410      559     +149     
Impacted Files Coverage Δ
...vlet/SentryRequestHttpServletRequestProcessor.java 94.73% <ø> (ø)
...n/java/io/sentry/spring/boot/SentryProperties.java 74.07% <ø> (-2.60%) ⬇️
.../io/sentry/spring/SentryInitBeanPostProcessor.java 97.36% <ø> (-0.31%) ⬇️
...racing/SentrySpanClientHttpRequestInterceptor.java 0.00% <0.00%> (ø)
.../sentry/DuplicateEventDetectionEventProcessor.java 100.00% <ø> (ø)
sentry/src/main/java/io/sentry/EventProcessor.java 50.00% <ø> (ø)
sentry/src/main/java/io/sentry/HubAdapter.java 4.91% <0.00%> (-0.09%) ⬇️
...ntry/src/main/java/io/sentry/NoOpSentryClient.java 70.00% <0.00%> (+10.00%) ⬆️
...main/java/io/sentry/NoOpSentryExecutorService.java 40.00% <0.00%> (-10.00%) ⬇️
...entry/src/main/java/io/sentry/NoOpTransaction.java 26.66% <ø> (+8.48%) ⬆️
... and 139 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...5732ed1. Read the comment docs.

@stefanosiano
Copy link
Member Author

Review not required to merge... 🤔 😈

@marandaneto
Copy link
Contributor

@stefanosiano can you point us to which commits actually implement the serialization/deserialization of the new protocol? otherwise, it's hard to review.

@marandaneto
Copy link
Contributor

Review not required to merge... 🤔 😈

Such rules exist only in main

@stefanosiano
Copy link
Member Author

@marandaneto Sure, the json commit is c860b8c
The other commit for v6 is 5732ed1, which contains very minor changes to make it compatible with v6 (e.g. IBuildInfoProvider -> BuildInfoProvider)

@stefanosiano
Copy link
Member Author

@marandaneto Can i merge it or should i wait for your review?

@marandaneto
Copy link
Contributor

@marandaneto Can i merge it or should i wait for your review?

@romtsn would you like to take this one?

@romtsn
Copy link
Member

romtsn commented Apr 5, 2022

@marandaneto will do!

Copy link
Member

@romtsn romtsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few minor comments, but lgtm otherwise!

@@ -0,0 +1,16 @@
package io.sentry
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this file (and FileUtilsTest) are in the wrong folder (see Sentry with capital "S"). I think we shouldn't have this Sentry folder in the first place ;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is weird... there's no Sentry folder anywhere... maybe due to mac being case insensitive.
However, I fixed it with command line :/

CHANGELOG.md Outdated
* Feat: Add Android profiling traces #1897 and its tests #1949
- All operations involving file reads for profiling were moved to the background #1959

## 5.6.2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to rebase/merge on 6.x.x again, this change should not be here (i.e. only the profiling changelog is relevant)

@@ -0,0 +1 @@
25b45607-d437-496f-9610-fb43a692dd7c
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also should be gone from the PR I believe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants