Add poisson upscaling without storing count in the sample types #908
Conversation
This file contains 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
realFlowControl
force-pushed
the
florian/poisson-upscaling
branch
from
7bb80e6 to
f761446
Compare
BenchmarksComparisonBenchmark execution time: 2025-03-06 08:19:09 Comparing candidate commit b8ed15a in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 52 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
BaselineOmitted due to size. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #908 +/- ##
=======================================
Coverage 72.21% 72.21%
=======================================
Files 330 330
Lines 49065 49128 +63
=======================================
+ Hits 35430 35480 +50
- Misses 13635 13648 +13
🚀 New features to boost your workflow:
|
realFlowControl
force-pushed
the
florian/poisson-upscaling
branch
from
f761446 to
797805b
Compare
2 tasks
morrisonlevi
approved these changes
Mar 5, 2025
Comment on lines
+166
to
+170
| } => write!( | ||
| f, | ||
| "Poisson = sum_value_offset: {}, count_value: {}, sampling_distance: {}", | ||
| sum_value_offset, count_value, sampling_distance | ||
| ), |
There was a problem hiding this comment.
Clippy would probably suggest this:
Suggested change
| } => write!( | |
| f, | |
| "Poisson = sum_value_offset: {}, count_value: {}, sampling_distance: {}", | |
| sum_value_offset, count_value, sampling_distance | |
| ), | |
| } => write!( | |
| f, | |
| "Poisson = sum_value_offset: {sum_value_offset}, count_value: {count_value}, sampling_distance: {sampling_distance}" | |
| ), |
(or something close, writing code in a text box in a browser is hard)
This kind of thing is repeated a few times in this file (in and out of the PR).
There was a problem hiding this comment.
Interestingly clippy does not complain, but I see value in the proposed change.
I'd tend keep it as is for consistency reasons and clean up all of this in follow-up PR, WDYT?
realFlowControl
changed the title
realFlowControl
added a commit
that referenced
this pull request
Mar 6, 2025
Merged
realFlowControl
added a commit
that referenced
this pull request
Mar 6, 2025
* Add poisson upscaling without storing count in the sample types (#908) * prepare v15.1.0
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?
So far our poisson upscaling relied on the count of samples to be part of the sample data. This works fine for sample types where we also collect the count, like memory allocations, where we collect the
alloc-sizeandalloc-count. In that case we can just use thealloc-countfrom the samples collected. In some cases, like I/O profiling, we add a lot of sample types (socket/file read/write times/bytes) but also adding the count for all of these operations would just bloat the resulting pprof, without a lot of value, so we need a way to inject the count to calculate the scale without having it in a sample type. This PR adds this.Motivation
I/O profiling in PHP is such a feature, where we add 8 sample types already. Adding all the counts would add another 8 sample types for no value (besides upscaling, which we can solve differently).
Additional Notes
v15.1.0as the PHP profiler is not yet onv16and I do not want to mix to many things together.clippyerrors are unrelated to the changes in this PRHow to test the change?
DataDog/dd-trace-php#3118