refactor(profiling): backup sample types/period #403
Merged
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
morrisonlevi
force-pushed
the
levi/sample-types-on-reset
branch
from
12fae0b to
2ae7586
Compare
morrisonlevi
force-pushed
the
levi/alloc-crate
branch
from
09926b4 to
4ff260b
Compare
morrisonlevi
force-pushed
the
levi/sample-types-on-reset
branch
from
2ae7586 to
31e33c2
Compare
ivoanjo
reviewed
Apr 29, 2024
morrisonlevi
force-pushed
the
levi/sample-types-on-reset
branch
2 times, most recently
from
5a3f760 to
e212171
Compare
morrisonlevi
force-pushed
the
levi/alloc-crate
branch
from
e5524f2 to
1196378
Compare
morrisonlevi
force-pushed
the
levi/sample-types-on-reset
branch
2 times, most recently
from
f74b130 to
e102e07
Compare
morrisonlevi
force-pushed
the
levi/alloc-crate
branch
3 times, most recently
from
0f98317 to
622db33
Compare
morrisonlevi
force-pushed
the
levi/sample-types-on-reset
branch
3 times, most recently
from
6ae45a4 to
83b7fa1
Compare
morrisonlevi
force-pushed
the
levi/alloc-crate
branch
from
622db33 to
b68fe99
Compare
morrisonlevi
force-pushed
the
levi/sample-types-on-reset
branch
from
e9e34af to
a1f5a62
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #403 +/- ##
=======================================
Coverage ? 65.57%
=======================================
Files ? 187
Lines ? 22873
Branches ? 0
=======================================
Hits ? 15000
Misses ? 7873
Partials ? 0
|
ivoanjo
reviewed
May 8, 2024
| /// Despite our manual drop order, it is still recommended to order the | ||
| /// fields in the order things should be dropped. | ||
| storage: ManuallyDrop<Vec<(String, String)>>, | ||
| period: Option<((String, String), i64)>, |
There was a problem hiding this comment.
Minor: Out of curiosity, it seems that often we either omit or pass in a dummy period. I wonder if we should just get rid of this parameter for libdatadog API users (and libdatadog would always just insert a default one).
There was a problem hiding this comment.
I don't think "period" really makes sense anyway, because we smush everything into a single profile. Only really makes sense for certain sample types, and only then it depends on the implementation.
I think we can file this under "out of scope" for this PR?
There was a problem hiding this comment.
Yeap, for sure, more of a passing observation.
morrisonlevi
force-pushed
the
levi/sample-types-on-reset
branch
from
3883f89 to
fa7a21f
Compare
morrisonlevi
changed the title
This breaks a dependency on the string table implementation details to be able to reset a profile and preserve the sample types and period. It does cause more allocations, though. Adds api::ValueType::new and uses it instead of constructing it directly, because in most cases with `new` it becomes a single line instead of four.
morrisonlevi
force-pushed
the
levi/sample-types-on-reset
branch
from
fa7a21f to
724f019
Compare
danielsn
approved these changes
May 9, 2024
| /// Despite our manual drop order, it is still recommended to order the | ||
| /// fields in the order things should be dropped. | ||
| storage: ManuallyDrop<Vec<(String, String)>>, | ||
| period: Option<((String, String), i64)>, |
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 refactors the Profile to backup the sample types and period outside of the string table.
Adds
api::ValueType::new, and uses that instead of direct instantiation. In most cases, this shortens from 4 lines to 1 (but not always).Simplifies some
vec![...]usage to[...]in tests, since the vector isn't needed at all.Motivation
Restoring the sample types and period was the only place that used
fn get_string(&self, id: StringId) -> &stroutside of tests. Requiring the data structure to map in both directions to provide this functionality makes it hard for innovation. The upcomingStringTablePR will not provide theStringIdto&strfunctionality.Additional Notes
This is stacked on top of another PR, and there will be another PR stacked on this. They should all be merged at roughly the same time.
How to test the change?
This should be an internal change only, so test as usual.