PROF-9791: Use heuristics to start the profiler when requested through SSI #4322
Conversation
Overall package sizeSelf size: 6.54 MB Dependency sizes
🤖 This report was automatically generated by heaviest-objects-in-the-universe |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4322 +/- ##
===========================================
+ Coverage 18.61% 62.86% +44.24%
===========================================
Files 6 243 +237
Lines 736 10308 +9572
Branches 33 33
===========================================
+ Hits 137 6480 +6343
- Misses 599 3828 +3229 ☔ View full report in Codecov by Sentry. |
szegedi
force-pushed
the
szegedi/profiler-heuristics
branch
from
dd67713 to
1bdd8b6
Compare
BenchmarksBenchmark execution time: 2024-05-29 12:42:08 Comparing candidate commit 9ca48ce in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 260 metrics, 6 unstable metrics. |
szegedi
force-pushed
the
szegedi/profiler-heuristics
branch
3 times, most recently
from
62af38e to
96b13bb
Compare
szegedi
force-pushed
the
szegedi/profiler-heuristics
branch
from
96b13bb to
cddcbd6
Compare
Also make sure mock profiler event emitter runs until heuristics trigger.
szegedi
force-pushed
the
szegedi/profiler-heuristics
branch
from
cddcbd6 to
33003e2
Compare
szegedi
force-pushed
the
szegedi/profiler-heuristics
branch
from
33003e2 to
e76c79e
Compare
| @@ -663,6 +667,17 @@ class Config { | |||
| this._setBoolean(env, 'profiling.enabled', coalesce(DD_EXPERIMENTAL_PROFILING_ENABLED, DD_PROFILING_ENABLED)) | |||
| this._setString(env, 'profiling.exporters', DD_PROFILING_EXPORTERS) | |||
| this._setBoolean(env, 'profiling.sourceMap', DD_PROFILING_SOURCE_MAP && !isFalse(DD_PROFILING_SOURCE_MAP)) | |||
| if (DD_INJECTION_ENABLED) { | |||
There was a problem hiding this comment.
Is it ok to put logic here? this seems to stray from the simple this._setBoolean/Value sequence of statement.
There was a problem hiding this comment.
I think yes. There's some other if statements in the method already.
| if (DD_INJECTION_ENABLED.split(',').includes('profiler')) { | ||
| this._setBoolean(env, 'profiling.heuristicsEnabled', true) | ||
| } | ||
| if (DD_INTERNAL_PROFILING_LONG_LIVED_THRESHOLD) { |
There was a problem hiding this comment.
Having two different naming for the same thing (long lived / short lived threshold) feels weird.
There was a problem hiding this comment.
I know – it already was there as "short lived" for telemetry, but we usually mentioned a "long-lived" process so this felt more natural. I was vacillating on renaming the internal variable but then thought I might do it in a separate step later to keep this smaller. I might add a commit to fix this though.
| @@ -13,17 +11,16 @@ const EnablementChoice = { | |||
| MANUALLY_ENABLED: Symbol('SSITelemetry.EnablementChoice.MANUALLY_ENABLED'), | |||
| SSI_ENABLED: Symbol('SSITelemetry.EnablementChoice.SSI_ENABLED'), | |||
| SSI_NOT_ENABLED: Symbol('SSITelemetry.EnablementChoice.SSI_NOT_ENABLED'), | |||
| DISABLED: Symbol('SSITelemetry.EnablementChoice.MANUALLY_DISABLED') | |||
| DISABLED: Symbol('SSITelemetry.EnablementChoice.DISABLED') | |||
There was a problem hiding this comment.
There is a MANUALLY_DISABLED reference left in the file
What does this PR do?
With this change, when
DD_INJECTION_ENABLEDhasprofileramong its comma-separated values then profiler will be started as soon as the application both created a tracing span and has reached a 30 second lifetime. This will ensure SSI profiling enablement doesn't profile either tracing-unaware applications or those that run for only a short period of time.Motivation
Adding profiler integration to SSI.