Initialize Profiler only once for multiple start calls#3081
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| val featureSdkCore = sdkCore as FeatureSdkCore | ||
| initializeProfiler() | ||
| val profilingFeature = ProfilingFeature( |
There was a problem hiding this comment.
Enabling profiling now eagerly calls initializeProfiler() before registering the feature, which constructs PerfettoProfiler (uses ProfilingManager/ProfilingResult) immediately. On devices below Build.VERSION_CODES.VANILLA_ICE_CREAM, calling Profiling.enable will now try to load those Android-15 classes and crash with NoClassDefFoundError/verify errors, whereas before enable left the NoOpProfiler in place and only created PerfettoProfiler when start ran. Consider guarding the initialization by API level or deferring it until profiling is actually supported.
Useful? React with 👍 / 👎.
|
🎯 Code Coverage 🔗 Commit SHA: f9809b4 | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
07a20ca
0357e6e to
07a20ca
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feature/perfetto-profiling #3081 +/- ##
==============================================================
+ Coverage 71.25% 71.28% +0.04%
==============================================================
Files 893 893
Lines 32694 32698 +4
Branches 5495 5496 +1
==============================================================
+ Hits 23293 23308 +15
+ Misses 7838 7827 -11
Partials 1563 1563
🚀 New features to boost your workflow:
|
| // Given | ||
| val sdkInstanceNames = setOf(fakeInstanceName) | ||
|
|
||
| Mockito.mockStatic(ProfilingStorage::class.java) |
There was a problem hiding this comment.
Is this needed? There is no work with ProfilingStorage inside the block.
There was a problem hiding this comment.
yes, Profiling.start will call ProfilingStorage.removeProfilingFlag(context, sdkInstanceNames) inside.
There was a problem hiding this comment.
This can also be avoided by simply making ProfilingStorage.sharedPreferencesStorage as internal instead of private and simply setting a mock there.
And by doing this we don't need to mock static.
07a20ca to
47bb279
Compare
|
|
||
| @Test | ||
| fun `M use PerfettoProfiler W enable called but start not called`() { | ||
| Mockito.mockStatic(ProfilingStorage::class.java) |
There was a problem hiding this comment.
Mockito.mockStatic(ProfilingStorage::class.java) calls here and below are not needed due to the ProfilingStorage.sharedPreferencesStorage = mockSharedPreferencesStorage call in the setup.
47bb279 to
f9809b4
Compare
500378e
into
feature/perfetto-profiling
5 participants
What does this PR do?
This PR takes one step further to support custom profiling.
The current implementation of
Profiling.ktis based on application launch profiling, which presumes theProfiling.startis called only once and always called beforeProfiling.enable.Which brings following consequences for custom profiling:
Profiling.enableis called beforeProfiling.start, thenProfilingFeaturewill be initialized withNoOpProfilerProfiling.startis called several times, the profiler will be recreated each times, which will be different from the instance held insideProfilingFeatureReview checklist (to be filled by reviewers)