[profiling] use internal Label instead of pprof::Label #454
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #454 +/- ##
==========================================
- Coverage 67.90% 67.89% -0.02%
==========================================
Files 193 193
Lines 24651 24643 -8
==========================================
- Hits 16740 16731 -9
- Misses 7911 7912 +1
|
|
I believe this means we can also remove |
| .map(|res| res.map(pprof::Label::from)), | ||
| ) | ||
| .chain(timestamp.map(|ts| Ok(Label::num(self.timestamp_key, ts.get(), None).into()))) | ||
| .map(|l| self.get_label(*l).copied()) |
There was a problem hiding this comment.
Thought: since Label is Copy, should get_label just return an owned object rather than a reference?
There was a problem hiding this comment.
(not needed for this PR, but might be interesting to not bother taking Copy types by reference
There was a problem hiding this comment.
I know for small types, yes, it's definitely preferred to work by-value instead of reference. I'm not sure what "small" means for Rust calling conventions, though. Is Label small?
There was a problem hiding this comment.
Label has StringId, which is u32 and LabelValue.
libdatadog/profiling/src/internal/label.rs
Lines 16 to 19 in 633b0b0
LabelVale is an enum which can be
StringId, againu32i64and optionalStringId
| label.get_key(), | ||
| match label.get_value() { | ||
| LabelValue::Str(str) => *str, | ||
| LabelValue::Num { .. } => StringId::ZERO, |
There was a problem hiding this comment.
Is this an error if this case is reached?
There was a problem hiding this comment.
I simply looked at impl From<&Label> for pprof::Label here.
libdatadog/profiling/src/internal/label.rs
Lines 59 to 73 in 0593cd7
I don't think this was treated as an error before this PR.
taegyunkim
changed the title
What does this PR do?
Resolves a TODO. Use
internal::Labelinstead ofpprof::Labelto make it easier to fuzz test. Even thoughpprof::Labelcan also derivebolero_generator::TypeGeneratorguarded bycfg(test), it would be best to keep internal things internal.Also, this lets us use
pprof::Labelonly when returning pprof.Motivation
TODO
Additional Notes
Anything else we should know when reviewing?
How to test the change?
cargo nextest run