[PROF-11456] Add extra validations for incorrect profiling location ids #902
+60
−20
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
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #902 +/- ##
==========================================
- Coverage 71.77% 71.76% -0.01%
==========================================
Files 328 328
Lines 48702 48722 +20
==========================================
+ Hits 34955 34965 +10
- Misses 13747 13757 +10
|
BenchmarksComparisonBenchmark execution time: 2025-02-27 11:50:14 Comparing candidate commit 56ab77b in PR branch Found 1 performance improvements and 0 performance regressions! Performance is the same for 51 metrics, 2 unstable metrics. scenario:benching deserializing traces from msgpack to their internal representation
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. |
danielsn
reviewed
Feb 27, 2025
| @@ -66,13 +67,34 @@ pub trait Dedup<T: Item> { | |||
| /// Panics if the number of items overflows the storage capabilities of | |||
| /// the associated Id type. | |||
| fn dedup(&mut self, item: T) -> <T as Item>::Id; | |||
|
|
|||
| /// Deduplicate the Item, and check if the generated Id is valid. | |||
| fn checked_dedup(&mut self, item: T) -> anyhow::Result<<T as Item>::Id>; | |||
There was a problem hiding this comment.
Having a panic in the code isn't great. Depending on how many places dedup is used in, maybe this should be the only version and clients can ? on it?
There was a problem hiding this comment.
I hesitated on doing that -- that ? will need to propagate quite far. Not sure it's worth the extra work given id overflow would mean we have 2**32 items so we'd already be using many GiB's for the profiler?
| @@ -1062,7 +1080,7 @@ mod api_tests { | |||
| } | |||
|
|
|||
| #[test] | |||
| fn lazy_endpoints() -> anyhow::Result<()> { | |||
| fn lazy_endpoints() { | |||
There was a problem hiding this comment.
It doesn't look correct for a test to exit early if something goes wrong: It should blow up?
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?
This PR adds extra validations for incorrect location ids being generated and/or being present at serialization time.
Motivation
In incident 35390 (JIRA PROF-11456) we observed invalid
location_ids being present in emitted profiles.We've stared hard at the code and can't really see how they could ever happen. By adding these checks, if we see the issue come up again, then we'll be sure that it happened after all the checks passed.
Additional Notes
N/A
How to test the change?
Since we don't know how to trigger the incorrect ids -- and they may not happen ever, it's hard to exercise these branches; yet, they're quite simple so I think it's fine to live with them as-is.