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

Fix benchmark issues #3099

Closed
wants to merge 13 commits into from
Closed

Fix benchmark issues #3099

wants to merge 13 commits into from

Conversation

jbertran
Copy link
Contributor

@jbertran jbertran commented May 4, 2023

What does this PR do?

Service naming (#2941) is the culprit for the recent benchmark issues, this PR tries to identify the cause and fix it.

@jbertran jbertran requested a review from a team as a code owner May 4, 2023 09:13
@github-actions
Copy link

github-actions bot commented May 4, 2023

Overall package size

Self size: 4.13 MB
Deduped: 58.26 MB
No deduping: 58.31 MB

Dependency sizes

name version self size total size
@datadog/pprof 2.2.1 14.24 MB 15.12 MB
@datadog/native-iast-taint-tracking 1.4.1 14.85 MB 14.86 MB
@datadog/native-appsec 3.1.0 13.31 MB 13.32 MB
protobufjs 7.1.2 2.76 MB 6.55 MB
@datadog/native-iast-rewriter 2.0.1 2.09 MB 2.1 MB
@datadog/native-metrics 2.0.0 898.77 kB 1.3 MB
opentracing 0.14.7 194.81 kB 194.81 kB
semver 7.3.8 88.2 kB 118.6 kB
@datadog/sketches-js 2.1.0 109.9 kB 109.9 kB
lodash.sortby 4.7.0 75.76 kB 75.76 kB
lru-cache 7.14.0 74.95 kB 74.95 kB
ipaddr.js 2.0.1 59.52 kB 59.52 kB
ignore 5.2.0 48.87 kB 48.87 kB
import-in-the-middle 1.3.5 34.34 kB 38.81 kB
istanbul-lib-coverage 3.2.0 29.34 kB 29.34 kB
retry 0.10.1 27.44 kB 27.44 kB
lodash.uniq 4.5.0 25.01 kB 25.01 kB
limiter 1.1.5 23.17 kB 23.17 kB
lodash.kebabcase 4.1.1 17.75 kB 17.75 kB
lodash.pick 4.4.0 16.33 kB 16.33 kB
node-abort-controller 3.0.1 14.33 kB 14.33 kB
crypto-randomuuid 1.0.0 11.18 kB 11.18 kB
diagnostics_channel 1.1.0 7.07 kB 7.07 kB
path-to-regexp 0.1.7 6.78 kB 6.78 kB
koalas 1.0.2 6.47 kB 6.47 kB
methods 1.1.2 5.29 kB 5.29 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
Copy link

codecov bot commented May 4, 2023

Codecov Report

Merging #3099 (db835c6) into master (cad3d92) will decrease coverage by 0.06%.
The diff coverage is 59.18%.

@@            Coverage Diff             @@
##           master    #3099      +/-   ##
==========================================
- Coverage   86.92%   86.86%   -0.06%     
==========================================
  Files         318      327       +9     
  Lines       11630    11810     +180     
  Branches       33       33              
==========================================
+ Hits        10109    10259     +150     
- Misses       1521     1551      +30     
Impacted Files Coverage Δ
packages/dd-trace/src/plugins/outgoing.js 45.45% <0.00%> (-4.55%) ⬇️
packages/dd-trace/src/plugins/tracing.js 55.55% <16.66%> (-7.78%) ⬇️
.../dd-trace/src/service-naming/schemas/definition.js 28.57% <28.57%> (ø)
packages/dd-trace/src/service-naming/schemas/v0.js 42.85% <42.85%> (ø)
packages/dd-trace/src/service-naming/schemas/v1.js 50.00% <50.00%> (ø)
packages/dd-trace/src/service-naming/index.js 71.42% <71.42%> (ø)
packages/datadog-plugin-rhea/src/consumer.js 100.00% <100.00%> (ø)
packages/datadog-plugin-rhea/src/producer.js 100.00% <100.00%> (ø)
packages/dd-trace/src/config.js 98.98% <100.00%> (+0.04%) ⬆️
packages/dd-trace/src/plugin_manager.js 98.46% <100.00%> (+0.04%) ⬆️
... and 1 more

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@pr-commenter
Copy link

pr-commenter bot commented May 4, 2023

Benchmarks

Comparing candidate commit db835c6 in PR branch jbertran/fix-svc-naming-perfs with baseline commit cad3d92 in branch master.

Found 0 performance improvements and 13 performance regressions! Performance is the same for 432 metrics, 27 unstable metrics.

scenario:log-without-log-16

  • 🟥 execution_time [+9.859ms; +10.583ms] or [+5.743%; +6.164%]

scenario:log-with-error-16

  • 🟥 execution_time [+9.924ms; +11.547ms] or [+5.721%; +6.657%]

scenario:log-skip-log-16

  • 🟥 execution_time [+10.155ms; +11.099ms] or [+5.857%; +6.402%]

scenario:log-with-debug-16

  • 🟥 execution_time [+8.740ms; +11.151ms] or [+5.013%; +6.396%]

scenario:startup-with-tracer-16

  • 🟥 execution_time [+9.705ms; +10.537ms] or [+6.065%; +6.585%]

scenario:spans-finish-immediately-16

  • 🟥 cpu_user_time [+29.138ms; +35.268ms] or [+7.492%; +9.069%]
  • 🟥 execution_time [+33.356ms; +36.181ms] or [+7.981%; +8.657%]

scenario:log-without-log-18

  • 🟥 execution_time [+9.007ms; +13.143ms] or [+5.100%; +7.442%]

scenario:log-with-debug-18

  • 🟥 cpu_user_time [+7.538ms; +14.845ms] or [+5.034%; +9.913%]
  • 🟥 execution_time [+10.291ms; +11.167ms] or [+5.810%; +6.304%]

scenario:log-with-error-18

  • 🟥 execution_time [+9.918ms; +11.764ms] or [+5.589%; +6.629%]

scenario:log-skip-log-18

  • 🟥 execution_time [+8.996ms; +11.102ms] or [+5.042%; +6.222%]

scenario:startup-with-tracer-18

  • 🟥 execution_time [+9.794ms; +10.390ms] or [+5.912%; +6.272%]

@jbertran jbertran force-pushed the jbertran/fix-svc-naming-perfs branch from 0cc431e to 3d82e16 Compare May 4, 2023 10:23
@jbertran jbertran marked this pull request as draft May 4, 2023 14:46
@jbertran jbertran force-pushed the jbertran/fix-svc-naming-perfs branch 6 times, most recently from 9cda7c6 to 59a082a Compare May 5, 2023 14:46
@tlhunter tlhunter force-pushed the jbertran/fix-svc-naming-perfs branch from 6949b05 to 0a57123 Compare May 11, 2023 16:38
@tlhunter
Copy link
Member

Maybe try commenting out just the changes in plugin manager, test that, comment out just hte changes in config, test that, etc.

Tests will of course fail but see if the benchmark results improve.

@tlhunter
Copy link
Member

Hopefully #3150 fixes the log benchmark issues. I haven't yet tried to fix the startup issues.

@tlhunter tlhunter force-pushed the jbertran/fix-svc-naming-perfs branch from 24cc601 to db835c6 Compare May 12, 2023 18:54
@tlhunter
Copy link
Member

I'm rebasing on master now that some benchmark changes landed. Hopefully the log benchmark issues disappear.

@tlhunter tlhunter marked this pull request as ready for review May 12, 2023 18:57
@tlhunter
Copy link
Member

tlhunter commented May 12, 2023

Apparently #3150 had no affect?! The results are the same.

@jbertran
Copy link
Contributor Author

jbertran commented May 15, 2023

Comparing with the previous benchmark, we've removed cpu_user_time ballooning for most of the log benchmarks. #3146, which I've also rebased, exhibits the same behaviour. I guess this would point towards the performance hit spending significant time in system calls, where a log spends most of its time in user calls?

The interesting part for the log benchmark is that the without-log scenario is showing the same performance hit as the rest, so it's possible that none of the scenarios actually log messages for some reason.

@jbertran jbertran mentioned this pull request May 16, 2023
@jbertran
Copy link
Contributor Author

We're reasonably certain the benchmarks aren't representative. This is no longer needed.

@jbertran jbertran closed this May 16, 2023
@tlhunter tlhunter deleted the jbertran/fix-svc-naming-perfs branch January 19, 2024 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants