feat(profiling): add extensible profile type API for prototyping#2202
Draft
r1viollet wants to merge 3 commits into
Draft
feat(profiling): add extensible profile type API for prototyping#2202r1viollet wants to merge 3 commits into
r1viollet wants to merge 3 commits into
Conversation
Contributor
📚 Documentation Check Results📦
|
Contributor
Clippy Allow Annotation ReportComparing clippy allow annotations between branches:
Summary by Rule
Annotation Counts by File
Annotation Stats by Crate
About This ReportThis report tracks Clippy allow annotations for specific rules, showing how they've changed in this PR. Decreasing the number of these annotations generally improves code quality. |
|
r1viollet
commented
Jul 7, 2026
| /// [`ddog_prof_Profile_new_custom`]. | ||
| #[repr(C)] | ||
| #[derive(Copy, Clone)] | ||
| pub struct CustomPeriod<'a> { |
Contributor
Author
There was a problem hiding this comment.
not sure we need this, I'll have to check what periods mean in OTel
Contributor
Author
There was a problem hiding this comment.
So after checking
- pprof only has one period, so this does not really need to exist
- otel has one period per sample type (as this is split per sample type), so if you want to profile every X system call, you might want to specify it.
Contributor
🔒 Cargo Deny Results📦
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7888ea198
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
r1viollet
force-pushed
the
r1viollet/custom-profile-types
branch
from
f7888ea to
415afc9
Compare
r1viollet
commented
Jul 7, 2026
| WallLegacy, | ||
|
|
||
| // Experimental sample types for testing and development. | ||
| ExperimentalCount, |
Contributor
Author
There was a problem hiding this comment.
I was going to remove these, though this is an API breaking change, so I can move this to a separate PR.
Allow callers to create profiles with arbitrary (type, unit) string pairs without adding a variant to SampleType or cutting a libdatadog release. - Internal Profile storage changed from Box<[SampleType]> to Box<[ValueType<'static>]>, making the enum a pure conversion layer - Profile::try_new_with_value_types / try_new_with_value_types_and_dictionary added as the Rust raw-string path - ddog_prof_Profile_new_custom + CustomValueType / CustomPeriod structs added to the C FFI - Profile::create_with_value_types added to the CXX bridge - Replayer updated to round-trip unknown types via Box::leak (short-lived tool) - ExperimentalCount / ExperimentalNanoseconds / ExperimentalBytes removed: they mapped to fixed strings and could not distinguish multiple custom measurements of the same unit - examples/ffi/custom_profile_types.c demonstrates the new C API - Four new unit tests cover round-trip serialization, reset survival, value-count validation, and parity with the SampleType path
r1viollet
force-pushed
the
r1viollet/custom-profile-types
branch
from
415afc9 to
3b474e1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84771716f4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+584
to
+585
| let type_: &'static str = Box::leak(type_.to_string().into_boxed_str()); | ||
| let unit: &'static str = Box::leak(unit.to_string().into_boxed_str()); |
There was a problem hiding this comment.
When CXX callers repeatedly create and drop custom-value profiles, such as recreating a profile from configuration, Box::leak makes every type/unit and period string process-lifetime even after the Profile is dropped. Because the internal profile only keeps &'static str, these allocations are never reclaimed, so memory grows with each profile creation rather than merely copying the strings for construction; keep the owned strings with the profile/internal config or otherwise free them on drop.
Useful? React with 👍 / 👎.
r1viollet
commented
Jul 7, 2026
| return api::ValueType::from(st); | ||
| } | ||
| // Slow path: custom type not yet in the enum – own the strings. | ||
| let ty: &'static str = Box::leak(type_str.to_string().into_boxed_str()); |
Contributor
Author
There was a problem hiding this comment.
This is OK unless we continuously create new profile types, which should not happen ?
Contributor
Author
|
So Daniel had an idea to avoid the current change in constructor, we can have custom types that we can specify |
danielsn
approved these changes
Jul 7, 2026
danielsn
left a comment
danielsn
left a comment
Contributor
There was a problem hiding this comment.
As we discussed offline, a cleaner solution could be to add Custom0 .. CustomN enums and an API for associating them with a name and value type
| /// Deprecated: use `Profile::try_new_with_value_types` with a descriptive raw `(type, unit)` | ||
| /// pair instead. This fixed name cannot distinguish multiple custom time measurements. | ||
| ExperimentalNanoseconds, | ||
| /// Deprecated: use `Profile::try_new_with_value_types` with a descriptive raw `(type, unit)` |
Contributor
There was a problem hiding this comment.
Is anyone currently using this or can we just remove it?
Contributor
Author
There was a problem hiding this comment.
I wanted to remove these, and then I had semver check issues 🤔
I don't think they are used, I'd favour removing them
Use Daniel's slot-based design for custom profile types: callers create profiles with SampleType::Custom1 through Custom5, then configure each slot with its concrete (type, unit) pair before serialization. This keeps the normal profile constructors as the primary API while still allowing profiler teams to prototype new profile types before promoting them to stable SampleType variants. BREAKING CHANGE: remove the ExperimentalCount, ExperimentalNanoseconds, and ExperimentalBytes SampleType variants in favor of Custom1 through Custom5.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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?
Adds five configurable custom profile type slots for profile types that are not yet stable enough to add as dedicated
SampleTypevariants.SampleType::Custom1throughSampleType::Custom5(type, unit)pair before serializationProfile::set_custom_sample_typeddog_prof_Profile_set_custom_sample_typeProfile::set_custom_sample_type(type, unit)pairs ontoCustom1..Custom5Experimental*variantsmemory-breakdown/bytesMotivation
#1450 moved the C API toward
SampleType, which is still the right API for stable, agreed-upon profile types.This PR adds @danielsn's slot-based prototyping path: profilers can use a bounded set of custom slots, configure their names/units at profile construction time, and avoid waiting for a libdatadog release while evaluating a new type such as
memory-breakdown. Once the type is stable, it should be promoted to a dedicatedSampleTypevariant and callers should migrate back to the fully typed API.Additional Notes
The old
ExperimentalCount,ExperimentalNanoseconds, andExperimentalBytesvariants were fixed to names likeexperimental-bytes, so they could not express a meaningful type name and only provided three unit-specific slots. They are replaced byCustom1..Custom5.For
memory-breakdown, the sample type is configured as("memory-breakdown", "bytes"). Anonymous/file/JIT/etc. breakdowns are represented by separate stacks or labels within that type, not by separate sample types.The period remains profile-level sampling metadata, not per-sample-type metadata. The custom example passes no period because
memory-breakdownis a point-in-time byte measurement with no meaningful sampling cadence.BREAKING CHANGE:
ExperimentalCount,ExperimentalNanoseconds, andExperimentalBytesare removed fromSampleType. UseCustom1..Custom5and configure the selected slot withset_custom_sample_type.How to test the change?
Local macOS:
cargo +nightly-2026-02-08 fmt --all -- --check cargo check -p libdd-profiling --features cxx cargo test -p libdd-profiling -p libdd-profiling-ffi -p datadog-profiling-replayer --no-fail-fast cargo +stable clippy --workspace --all-targets --all-features -- -D warningsLinux workspace: