-
Notifications
You must be signed in to change notification settings - Fork 15
fix: Consume Observations on drop of iterator
#1362
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
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1362 +/- ##
==========================================
+ Coverage 71.19% 71.21% +0.01%
==========================================
Files 392 392
Lines 62677 62742 +65
==========================================
+ Hits 44626 44679 +53
- Misses 18051 18063 +12
🚀 New features to boost your workflow:
|
|
We are seeing this error occur sporadically since updating dd-tracer-php to 1.14.0, and are unable to track down the root cause or easily replicate. Following. |
|
Hey @idanoo, |
6748986 to
a75421d
Compare
BenchmarksComparisonBenchmark execution time: 2025-11-24 11:28:24 Comparing candidate commit a75421d in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 55 metrics, 2 unstable metrics. CandidateCandidate benchmark detailsGroup 1
Group 2
Group 3
Group 4
Group 5
Group 6
Group 7
Group 8
Group 9
Group 10
Group 11
Group 12
Group 13
Group 14
Group 15
Group 16
Group 17
BaselineOmitted due to size. |
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-apple-darwin
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-apple-darwin
x86_64-unknown-linux-gnu
|
a75421d to
214251e
Compare
|
/merge |
|
View all feedbacks in Devflow UI.
This pull request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
The expected merge time in
|
consume observations on drop of iterator add test for anyhow bailout `upscale_values` can not fail Add missing comment Co-authored-by: florian.engelhardt <florian.engelhardt@datadoghq.com>
Observations on drop of iterator
What does this PR do?
Add an iterator that will consume the rest of the iterator on drop. Also changes the
upscale_valuesfunction signature, as this function can't fail.Motivation
We have seen crashes because of this assertion:
libdatadog/libdd-profiling/src/internal/observation/trimmed_observation.rs
Lines 113 to 119 in 8e56742
This can happen if this loop bails out early:
libdatadog/libdd-profiling/src/internal/profile/mod.rs
Lines 418 to 450 in 8e56742
If it does, the
iterwill be dropped at the end of the loop, dropping the unconsumedTrimmedObservation's which will fail the assertion and as such burry theanyhow::Resultand we'll not know what happened to return early and crash the customer application.Additional Notes
Anything else we should know when reviewing?
How to test the change?
$ cargo test reproduce_crash_with_anyhow_bailout