-
Notifications
You must be signed in to change notification settings - Fork 10
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
[profiling] Fuzz profiler API #449
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #449 +/- ##
==========================================
+ Coverage 68.93% 69.36% +0.42%
==========================================
Files 193 194 +1
Lines 25574 26006 +432
==========================================
+ Hits 17630 18039 +409
- Misses 7944 7967 +23
|
2a1d752
to
900d192
Compare
3971f7a
to
af75caa
Compare
bf22384
to
385f554
Compare
c9dc1f5
to
dc8a9df
Compare
027ca3e
to
db18e70
Compare
} | ||
} | ||
|
||
/// PartialEq and Hash work on just the key to avoid getting duplicate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we using Eq
to compare labels in the tests? Will this miss issues where the values don't agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::hash::Hash
doc says this property must hold when implementing Eq
and Hash
, k1 == k2 -> hash(k1) == hash(k2)
. In our case, values can disagree even if the hashes are the same.
This was introduced mainly to use Labels as keys for HashMap in https://github.com/DataDog/libdatadog/blob/dsn/fuzz-profile-api/profiling/src/internal/profile/fuzz_tests.rs#L369
if let Some(expected_sample) = expected_timestamped_samples.next() { | ||
assert_eq!(owned_locations, expected_sample.locations); | ||
assert_eq!(sample.values, expected_sample.values); | ||
assert_eq!(owned_labels, expected_sample.labels); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should potentially be sorted: there is no requirement that labels remain in the same order
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Locally I'm sorting the labels and got a new failure:
======================== Test Failure ========================
Input:
(
[],
[
(
None,
Sample {
locations: [],
values: [],
labels: [
Label {
key: "local root span id",
str: None,
num: 281474976710656,
num_unit: None,
},
Label {
key: "",
str: Some(
"",
),
num: 0,
num_unit: Some(
"",
),
},
],
},
),
],
)
Error:
panicked at profiling/src/internal/profile/fuzz_tests.rs:402:18:
Value not found for an aggregated sample
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
==============================================================
Line 402 is this expect:
let key = (&owned_locations, &owned_labels);
let expected_values = samples_without_timestamps
.get(&key)
.expect("Value not found for an aggregated sample");
assert_eq!(&sample.values, expected_values);
I think this happens because Vec<Label>
and such are used as keys in the hash, and then they don't work out because of order?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed this up with function fuzz_failure_001
to reproduce.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Levi for capturing this test case!
9cfe3b5
to
9ad1b94
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I don't know much about what a bolero_generator is, but the changes look fine 👍
I am guessing we have some amount of work to fix the tests before we can merge this ?
|
139f2bd
to
ef5762e
Compare
Yes, the test fails because input labels are not ordered in an expected way. The label with 'local root span id' should follow other labels, looking at this function. libdatadog/profiling/src/internal/profile/mod.rs Lines 493 to 497 in 6f37a28
This failing test case has been introduced with these code lines. libdatadog/profiling/src/internal/profile/fuzz_tests.rs Lines 231 to 247 in 6f37a28
I wanted to have bolero generated Samples to have a label with key 'local root span id', but didn't make sure that the orders are as expected. |
cc81644
to
288ff5c
Compare
What does this PR do?
Adds fuzzable versions of the data structures, and uses them to fuzz the profiler API.
Motivation
R&D week
Additional Notes
Anything else we should know when reviewing?
How to test the change?
Describe here in detail how the change can be validated.