profiler: randomize when execution traces are collected#2401
Merged
Conversation
BenchmarksBenchmark execution time: 2023-12-15 14:52:28 Comparing candidate commit b684ba8 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 39 metrics, 2 unstable metrics. |
nsrip-dd
commented
Nov 30, 2023
nsrip-dd
force-pushed
the
PROF-8728-randomize-execution-trace-collection
branch
from
December 1, 2023 15:46
a04d37d to
5239976
Compare
nsrip-dd
force-pushed
the
PROF-8728-randomize-execution-trace-collection
branch
from
December 13, 2023 19:04
5239976 to
0c6d7d7
Compare
nsrip-dd
marked this pull request as ready for review
December 13, 2023 19:32
nsrip-dd
force-pushed
the
PROF-8728-randomize-execution-trace-collection
branch
2 times, most recently
from
December 14, 2023 16:45
8441dfc to
928f0b9
Compare
felixge
approved these changes
Dec 15, 2023
felixge
left a comment
Member
There was a problem hiding this comment.
LGTM 🙇 . I reviewed the code and ran the test cases with -race -count 20.
We currently record execution traces at a fixed interval. This means that apps which are deployed across several instances simulatenously will have time periods where no instances have execution trace data. This also biases us against activity which occurs with a frequency which is harmonic with the trace collection frequency. To fully address this we would need to decouple execution trace collection from the normal profiling cycle. But as a first step, we should give every profiling cycle a chance of recording data. This commit does that: each profiling cycle we record an execution trace with probability (profiling period) / (trace period). This way we still maintain the same desired avarage data rate of ~one trace every 15 minutes by default. We have one special case, though: we want a trace for the first profile cycle to capture startup activity, since that's usually quite different than normal program activity. Inline the shouldTrace function into the one place it should actually be used. Prior to this commit, shouldTrace was called from two places: once to decide whether to trace, and once as a double-check right before starting the trace. The double-check is not particularly helpful, and if we're making a decision randomly, then checking twice means we'd have to win the "coin toss" twice to record data. So we inline the logic into a single place, right before scheduling a trace. This is tested by doing many "trials" of recording profiles with different execution trace configurations, seeing how many traces we get, and making assertions about the number we see based on the expected probability of tracing. On the one hand, the actual change is fairly simple so perhaps this level of testing is overkill. We also are deliberatly introducing a "flaky" test. On the other hand, the first draft of this change had a bug from calling shouldTrace twice, and the added test catches that quite consistently. The test could be even stronger but perhaps this is good enough to start.
nsrip-dd
force-pushed
the
PROF-8728-randomize-execution-trace-collection
branch
from
December 15, 2023 14:37
928f0b9 to
b684ba8
Compare
nsrip-dd
commented
Dec 15, 2023
nsrip-dd
left a comment
Contributor
Author
There was a problem hiding this comment.
Thanks Felix! Per our discussion, I added a special case to make sure we trace during startup, and updated the test case to reflect that.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Randomize execution trace collection. Each profiling cycle we record an
execution trace with probability (profiling period) / (trace period). This
way we still maintain the same desired avarage data rate of one trace every 15
minutes by default.
Inline the shouldTrace function into the one place it should actually be
used. Prior to this commit, shouldTrace was called from two places: once
to decide whether to trace, and once as a double-check right before
starting the trace. The double-check is not particularly helpful, and if
we're making a decision randomly, then checking twice means we'd have to
win the "coin toss" twice to record data. So we inline the logic into a
single place, right before scheduling a trace.
This is tested by doing many "trials" of recording profiles with
different execution trace configurations, seeing how many traces we get,
and making assertions about the number we see based on the expected
probability of tracing. On the one hand, the actual change is fairly
simple so perhaps this level of testing is overkill. We also are
deliberatly introducing a "flaky" test. On the other hand, the first
draft of this change had a bug from calling shouldTrace twice, and the
added test catches that quite consistently. The test could be even
stronger but perhaps this is good enough to start.
Motivation
We currently record execution traces at a fixed interval. This means
that apps which are deployed across several instances simulatenously
will have time periods where no instances have execution trace data.
This also biases us against activity which occurs with a frequency which
is harmonic with the trace collection frequency. To fully address this
we would need to decouple execution trace collection from the normal
profiling cycle. But as a first step, we should give every profiling
cycle a chance of recording data.
Reviewer's Checklist
For Datadog employees:
@DataDog/security-design-and-guidance.Unsure? Have a question? Request a review!