-
Notifications
You must be signed in to change notification settings - Fork 317
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
lazy load public api + profiling + crashtracking + span stats #5256
base: master
Are you sure you want to change the base?
Conversation
Overall package sizeSelf size: 8.68 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.4.0 | 29.44 MB | 29.44 MB | | @datadog/native-appsec | 8.4.0 | 19.25 MB | 19.26 MB | | @datadog/native-iast-taint-tracking | 3.3.0 | 13.77 MB | 13.78 MB | | @datadog/pprof | 5.5.1 | 9.79 MB | 10.17 MB | | protobufjs | 7.2.5 | 2.77 MB | 5.16 MB | | @datadog/native-iast-rewriter | 2.8.0 | 2.6 MB | 2.74 MB | | @opentelemetry/core | 1.14.0 | 872.87 kB | 1.47 MB | | @datadog/native-metrics | 3.1.0 | 1.06 MB | 1.46 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | import-in-the-middle | 1.11.2 | 112.74 kB | 835.4 kB | | source-map | 0.7.4 | 226 kB | 226 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | lru-cache | 7.18.3 | 133.92 kB | 133.92 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 kB | | @datadog/sketches-js | 2.1.0 | 109.9 kB | 109.9 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | ignore | 5.3.2 | 53.63 kB | 53.63 kB | | shell-quote | 1.8.1 | 44.96 kB | 44.96 kB | | istanbul-lib-coverage | 3.2.0 | 29.34 kB | 29.34 kB | | rfdc | 1.3.1 | 25.21 kB | 25.21 kB | | @isaacs/ttlcache | 1.4.1 | 25.2 kB | 25.2 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | dc-polyfill | 0.1.4 | 23.1 kB | 23.1 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | semifies | 1.0.0 | 15.84 kB | 15.84 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | ttl-set | 1.0.0 | 4.61 kB | 9.69 kB | | path-to-regexp | 0.1.12 | 6.6 kB | 6.6 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | module-details-from-path | 1.0.3 | 4.47 kB | 4.47 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5256 +/- ##
==========================================
- Coverage 81.29% 81.25% -0.05%
==========================================
Files 481 487 +6
Lines 21598 21708 +110
==========================================
+ Hits 17558 17638 +80
- Misses 4040 4070 +30 ☔ View full report in Codecov by Sentry. |
Datadog ReportBranch report: ✅ 0 Failed, 630 Passed, 0 Skipped, 15m 49.7s Total Time |
BenchmarksBenchmark execution time: 2025-02-13 22:10:17 Comparing candidate commit 21b9e4d in PR branch Found 5 performance improvements and 4 performance regressions! Performance is the same for 817 metrics, 22 unstable metrics. scenario:appsec-iast-startup-time-iast-enabled-20
scenario:exporting-pipeline-0.5-18
scenario:exporting-pipeline-0.5-20
scenario:exporting-pipeline-0.5-22
scenario:plugin-graphql-with-depth-and-collapse-on-18
scenario:plugin-graphql-with-depth-off-18
scenario:plugin-graphql-with-depth-on-max-18
|
const profilingEnabledEnv = coalesce( | ||
DD_EXPERIMENTAL_PROFILING_ENABLED, | ||
DD_PROFILING_ENABLED, | ||
this._isInServerlessEnvironment() ? 'false' : undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh, I see what you did here. Clever 😃.
For consistency, I'd probably move these to _applyCalculated
. I know it's not particularly interesting since you said we don't report telemetry but it's still conceptually cleaner. You could just put this in _applyCalculated()
:
if (this._isInServerlessEnvironment()) {
this._setValue(calc, 'profiling.enabled', 'false')
this._setValue(calc, 'crashtracking.enabled', false)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ironically, I moved them here for consistency, as other options are also calculated in this way in _applyEnv
. For now let's keep it there and we can revisit in a separate PR if needed, although I kind of think it makes sense to be in _applyEnv
as Serverless is an environment. In any case, I'd consider this out of scope of this PR since there are other occurrences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought "Env" in "_applyEnv" meant "environment variables". But you're right that some uses of specifically _isInServerlessEnvironment
have already snuck in, so by that reasoning I can accept these being there as well. If someone gets sufficiently irritated by it, they can fix it later.
8e9a33c
to
21b9e4d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good for profiling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm for llmobs
What does this PR do?
Lazy load non-tracing public API, profiling, crashtracking and span stats.
Motivation
These are all costly to import and increase startup time even when not needed.