dd-trace-php icon indicating copy to clipboard operation
dd-trace-php copied to clipboard

Published 20 hours ago • verified publisher icon DataDog

feat(profiling): emit metrics for stack-walking overhead

Open morrisonlevi opened this issue 2 years ago • 6 comments
trafficstars

Description

Currently emits a metric datadog.profiling.php.stack_walk_ns as a histogram. It's gated behind the feature profiling_metrics which is off by default.

This is needed to more accurately track the cost of stack walking.

Also fixes some clippy warnings.

PROF-7576

Readiness checklist

  • [ ] Changelog has been added to the release document.
  • [ ] Tests added for this feature/bug.

Reviewer checklist

  • [x] Appropriate labels assigned.
  • [x] Milestone is set.

morrisonlevi avatar May 03 '23 20:05 morrisonlevi

Benchmarks

Benchmark execution time: 2023-07-06 11:28:44

Comparing candidate commit 4737d98710b65255b4f6df5e7272fabbdc9ac6ff in PR branch levi/profiling-stats with baseline commit 3a37639fed88fc528e60153d42f1a644ccd64f29 in branch master.

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

pr-commenter[bot] avatar Jun 30 '23 14:06 pr-commenter[bot]

It's working 😄 I have two ideas that we should think about before GAing this (so it is not blocking this PR):

  • we could maybe move the dogstatsd.rs to libdatadog
  • we should measure the impact on sample creation, as currently this is in the hot path

realFlowControl avatar Jul 06 '23 11:07 realFlowControl

Do we need to turn this on for releases (not release builds, the releases we ship to customers)? My thought was we don't, and this lowers our risk substantially.

morrisonlevi avatar Jul 06 '23 22:07 morrisonlevi

I thought the same, that's why I approved it. Additionally I like the idea of somewhen in the future having our profiler sending those kind of data to us, so we would have more insight in how it performs on customer machines.

realFlowControl avatar Jul 07 '23 08:07 realFlowControl

I also think dogstatsd would be a good fit for a small crate in libdatadog. Can we move it there? :-)

bwoebi avatar Jul 08 '23 09:07 bwoebi

I worry if we put the dogstatsd bits into libdatadog that it may unintentionally get used in prod by libdatadog users. There is a tiny bit of testing but I'm not sure it's production ready. That doesn't matter for this PR, since it's not enabled for our production builds, but users in libdatadog may not realize this.

I do agree it would be nice to have a solid dogstatsd client in libdatadog, though.

morrisonlevi avatar Jul 15 '23 02:07 morrisonlevi