perf(profiling): reuse exporter across upload cycles - #17584
Conversation
Codeowners resolved as |
|
| // Drop the cached exporter before fork so the child doesn't inherit stale | ||
| // Rust state (tokio runtime, TLS, connection pool). Dropping here — in the | ||
| // parent, under lock, with no upload in flight — is safe. Both parent and | ||
| // child will lazily recreate the exporter on the next upload. |
There was a problem hiding this comment.
Doing some research allowed me to find that the original reason for not reusing the Exporter was exactly this: runtime_id changes as well as process_id (but this last one was introduced way later).
If we properly reinitialise the Exporter post-fork in the child, I guess we should be fine...
c1a0ac4 to
7e769f9
Compare
r1viollet
added
the
changelog/no-changelog
|
Now that we have reached a more stable state, I'll consider pushing these changes |
|
@codex review |
![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: 7e769f9c53
ℹ️ 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".
| if (state.cached_exporter.inner != nullptr) { | ||
| return std::nullopt; |
There was a problem hiding this comment.
When the tracer endpoint or per-upload tags change after the first upload, this early return keeps using the exporter that was built from the old ProfilerState snapshot. _ddup.upload() still calls ddup_set_runtime_id(), ddup_set_process_id(), ddup_config_user_tag("process_type", ...), and ddup_config_url() immediately before every ddup_upload(), so applications that reconfigure the tracer URL or whose runtime/process metadata changes without a fork will keep sending profiles with stale destination/tags until the next fork or process exit. Consider invalidating cached_exporter from the relevant set_* methods or comparing the current config before reusing it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I'm not sure how valid that is; clearly it would be the "most correct thing to do", but I don't know if there's a simple way to check whether the configuration has changed since the process start.
What I can think of:
- ✔️ I don't think the runtime ID should change
- ✔️ I don't think the PID should change -- we reset it post-fork so as long as we drop the Exporter post-fork and either re-construct it after setting the PID (or just lazily, at next upload) it should be fine
- ✔️ I don't think changing the upload URL mid-application lifetime is a real pattern used by anyone
⚠️ I do think the user tags thing is a valid concern... We've discussed "manual usage of theProfilerclass" in the past and initially thought we should be preventing it. In the meantime, we've (re-)discovered that it was a publicly advertised feature (in our docs) so we can't just kill it. Thing is, manualProfilerclass usage is useful among others in cases where the user wants to set specific tags based on application-level data (e.g. the Delancie Worker sets the Task name tag before starting its Profiler). In the current form, I think this may or may not break (depending on whether we are lucky, whether the parent process had an active Profiler, etc.)
So I would say:
- Either make sure we reconstruct the Exporter if user tags change (bit of a hassle and probably not-great code as we'll need additional variables to keep track of past state and compare, for sure)
- Either deliberately possibly break this -- but not always, again it's a matter of luck -- but given that we postponed the deprecation of manual
Profilerusage to the next major, I wouldn't do that -- it would look too accidental (+ we still have some internal use for it at the moment...)
KowalskiThomas
left a comment
KowalskiThomas
left a comment
There was a problem hiding this comment.
Sorry I took so long to review!
I think overall the changes make sense (despite my ten comments) but to be sure we're not missing anything I'd recommend testing on dogweb/dd-source staging or at the very least smoke test apps for a week or so.
We have docs on how to do that if you need it let me know!
| // Cached profile exporter, reused across upload cycles. Each construction | ||
| // allocates a tokio runtime, TLS connector and HTTP client on the Rust side | ||
| // (~10-15 native allocations), so we build it once and keep it. | ||
| // Lifetime: created lazily on first upload, dropped in prefork() (parent side, | ||
| // while tokio threads are still alive) and in cleanup(). Always accessed under | ||
| // upload_lock — except in cleanup(), which runs single-threaded at exit. | ||
| // Config snapshot: baked from ProfilerState at first build; later set_* calls | ||
| // do not retroactively update it. |
There was a problem hiding this comment.
| // Cached profile exporter, reused across upload cycles. Each construction | |
| // allocates a tokio runtime, TLS connector and HTTP client on the Rust side | |
| // (~10-15 native allocations), so we build it once and keep it. | |
| // Lifetime: created lazily on first upload, dropped in prefork() (parent side, | |
| // while tokio threads are still alive) and in cleanup(). Always accessed under | |
| // upload_lock — except in cleanup(), which runs single-threaded at exit. | |
| // Config snapshot: baked from ProfilerState at first build; later set_* calls | |
| // do not retroactively update it. | |
| // Cached profile exporter, reused across upload cycles. Each construction | |
| // allocates a tokio runtime, TLS connector and HTTP client on the Rust side | |
| // (~10-15 native allocations), so we build it once and keep it. | |
| // Lifetime: created lazily on first upload, dropped in prefork (parent side, | |
| // while tokio threads are still alive) and in cleanup. Always accessed under | |
| // upload_lock — except in cleanup, which runs single-threaded at exit. | |
| // Config snapshot: baked from ProfilerState at first build; later set_* calls | |
| // do not retroactively update it. |
| if (state.cached_exporter.inner != nullptr) { | ||
| return std::nullopt; |
There was a problem hiding this comment.
I'm not sure how valid that is; clearly it would be the "most correct thing to do", but I don't know if there's a simple way to check whether the configuration has changed since the process start.
What I can think of:
- ✔️ I don't think the runtime ID should change
- ✔️ I don't think the PID should change -- we reset it post-fork so as long as we drop the Exporter post-fork and either re-construct it after setting the PID (or just lazily, at next upload) it should be fine
- ✔️ I don't think changing the upload URL mid-application lifetime is a real pattern used by anyone
⚠️ I do think the user tags thing is a valid concern... We've discussed "manual usage of theProfilerclass" in the past and initially thought we should be preventing it. In the meantime, we've (re-)discovered that it was a publicly advertised feature (in our docs) so we can't just kill it. Thing is, manualProfilerclass usage is useful among others in cases where the user wants to set specific tags based on application-level data (e.g. the Delancie Worker sets the Task name tag before starting its Profiler). In the current form, I think this may or may not break (depending on whether we are lucky, whether the parent process had an active Profiler, etc.)
So I would say:
- Either make sure we reconstruct the Exporter if user tags change (bit of a hassle and probably not-great code as we'll need additional variables to keep track of past state and compare, for sure)
- Either deliberately possibly break this -- but not always, again it's a matter of luck -- but given that we postponed the deprecation of manual
Profilerusage to the next major, I wouldn't do that -- it would look too accidental (+ we still have some internal use for it at the moment...)
|
|
||
| // Disable copy constructor and copy assignment operator to avoid double-free | ||
| // of ddog_exporter | ||
| // Disable copy to avoid double-free of encoded_profile. |
There was a problem hiding this comment.
Seems like some comments have been changed but I'm not sure whether they should have been 🤨
There was a problem hiding this comment.
To me this is relevant change
| // Caller holds upload_lock, so this access is serialized with prefork/cleanup | ||
| // (the only places that drop the exporter). |
There was a problem hiding this comment.
| // Caller holds upload_lock, so this access is serialized with prefork/cleanup | |
| // (the only places that drop the exporter). | |
| // Caller holds upload_lock, so this access is serialized with prefork/cleanup | |
| // (the only places that drop the exporter) and other uploads. |
I think there is theoretically a case where we could try to start two parallel uploads, but the upload lock prevents it. Might be worth adding here.
| // The exporter is owned by ProfilerState and reused across uploads. | ||
| // Caller holds upload_lock, so this access is serialized with prefork/cleanup | ||
| // (the only places that drop the exporter). | ||
| auto& cached_exporter = ProfilerState::get().cached_exporter; |
There was a problem hiding this comment.
Do we need to specify it's cached? Why not just exporter?
| } | ||
| ddog_CancellationToken_drop(&new_cancel_clone_for_request); | ||
| ddog_prof_Exporter_drop(&ddog_exporter); | ||
| // cached_exporter intentionally NOT dropped: it is reused across uploads. |
There was a problem hiding this comment.
I think we can just remove that comment, outside the context of that PR it doesn't need to exist.
| Datadog::UploaderBuilder::build() | ||
| namespace { | ||
|
|
||
| // Create the cached exporter if it does not already exist. |
There was a problem hiding this comment.
| // Create the cached exporter if it does not already exist. | |
| // Create the exporter if it does not already exist. |
Here as well I don't think we should specify "cached". If the model we have is "we have one Exporter object that we keep forever" then there's no need to name it "cached Exporter" just like we don't name our sampling thread "cached sampling thread".
| { ExportTagKey::profiler_version, state.profiler_version }, | ||
| { ExportTagKey::process_id, state.process_id } | ||
| const std::vector<std::pair<Datadog::ExportTagKey, std::string_view>> tag_data = { | ||
| { Datadog::ExportTagKey::dd_env, state.dd_env }, |
There was a problem hiding this comment.
No strong opinion, but would a using namespace Datadog in the anonymous namespace make sense here? I don't see a reason not to.
| to_slice(family), | ||
| Datadog::to_slice("dd-trace-py"), | ||
| Datadog::to_slice(state.profiler_version), | ||
| Datadog::to_slice(g_language_name), |
There was a problem hiding this comment.
Why did we change from family to language name? Is there a difference?
|
|
||
| // Lazily create the exporter on first build. Subsequent builds reuse it. | ||
| // The caller (ddup_upload) holds upload_lock, so this is serialized with | ||
| // prefork() / cleanup() — the only places that drop the exporter. |
There was a problem hiding this comment.
| // prefork() / cleanup() — the only places that drop the exporter. | |
| // prefork / cleanup — the only places that drop the exporter. |
The profiler builds a new ddog_prof_ProfileExporter on every upload (~60s). Each construction allocates a tokio runtime, TLS connector and HTTP client on the Rust side — about 10–15 native allocations per cycle that the allocator never gets to coalesce. Cache the exporter in ProfilerState and reuse it across uploads, matching ddprof's pattern. Lifecycle: - Lazily created on first build, under upload_lock (held by ddup_upload). - Dropped in prefork() while still in the parent (under upload_lock, with no upload in flight) — dropping it post-fork in the child is unsafe because the tokio worker threads do not survive fork. - Dropped in cleanup() at exit. - Recreated lazily on the next upload in both parent and child. Uploader no longer owns the exporter; it reaches the cached one through ProfilerState in upload_unlocked() instead. This keeps Uploader's move semantics intact and confines the exporter's lifecycle to ProfilerState. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address review feedback: - User-defined tags (DD_TAGS, dd_trace_api.profiling.tag, the per-task tag pattern used by manual Profiler usage) can change between uploads. Baking them into a long-lived exporter risks sending stale tags after such a change. Move them out of the exporter and pass them per-send via optional_additional_tags, which libdatadog exposes for exactly this case. The exporter still bakes the static identity tags (env, service, version, language, runtime, runtime_id, runtime_version, profiler_version, process_id) and the endpoint (URL, timeout). runtime_id and process_id remain correct after fork because we drop the exporter in prefork. - Rename ProfilerState::cached_exporter to ProfilerState::exporter; drop "cached" from associated comments and the local in upload_unlocked. - Lift the using-directive into the anonymous namespace in uploader_builder.cpp to remove the Datadog:: prefix noise. - Comment polish: no parens on function references (prefork, cleanup), expand the upload_unlocked comment to mention upload_lock also serializing parallel uploads, drop the trailing "intentionally NOT dropped" comment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…language distinction - Introduce g_family_name constant and use it for the exporter's family slot, separate from g_language_name. They coincide for this profiler, but the concepts are different (an eBPF profiler can run in family `native` while sampling `python` code). - Replace the literal "dd-trace-py" string with the existing g_library_name constant for the library-name slot. - New tag_rotation_program.py driver: starts a Profiler, rotates a per-upload tag (phase=setup/warmup/production), forces an upload each cycle. - New test_per_upload_tags.py: runs the driver under an in-process ThreadingHTTPServer mock, captures the multipart bodies, asserts each cycle's body contains the expected `phase:<value>` tag, AND asserts the previous cycle's tag does NOT appear in a later cycle's body. The negative assertion is the regression guard — if user tags were baked back into the cached exporter, the first cycle's tag would leak into all subsequent cycles' bodies. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…driver Profiler.start() takes no arguments in current main; the kwarg was a leftover from earlier iterations. Verified end-to-end against a real trace-agent on linux and via the mock-agent test_per_upload_tags.py (3 cycles, each with its own phase:<value> tag, no leakage).
…follow-up Reviewer flagged that UploaderBuilder::build() reaches into the ProfilerState singleton instead of taking what it needs as an argument. That pattern predates this PR (every set_* method + build_user_tag_vec already does it), and untangling it cleanly means threading an explicit config struct through the whole UploaderBuilder API surface. That is its own refactor; punting via an AIDEV-NOTE so the follow-up has a clear anchor.
81face1 to
72881f3
Compare
|
This pull request has been automatically closed after a period of inactivity. |
|
Superseded by #19330 — same 5 commits, rebased on current |
|
@r1viollet can we close the other PR and reopen this one to keep the review history etc. instead of everything vanishing in the new PR? |
|
@KowalskiThomas it does not look like it. Reopen button is not available. |
|
@r1viollet I believe that's because the other PR is open, if you close it you'll be able to re-open this one I think |
|
No, I tried this. Reopen button is still not complying (because the branch moved). |
2 participants
Description
The profiler was creating a new ddog_prof_ProfileExporter on every upload cycle (every ~60s), then immediately dropping it. Each creation allocates TLS connectors, tokio runtime state, and HTTP client resources in Rust — ~10-15 native allocations per cycle that contribute to memory fragmentation.
This follows the pattern used by ddprof, which creates the exporter once at startup and reuses it for all subsequent uploads.
Changes:
Testing
This is heavily vibe coded, so I would appreciate if someone from the python team could carefully review these code paths considering how dangerous the forking scenarios can be.
Risks
Additional Notes