perf(profiling): reuse exporter across upload cycles - #19330
Closed
r1viollet wants to merge 5 commits into
Closed
Conversation
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.
Contributor
|
Circular import analysis
|
Codeowners resolved as |
Contributor
Author
|
I did not find time to test this, however I want to keep it in the back of my mind. |
Contributor
Author
|
reopening previous PR |
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
1 participant
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.
Description
Supersedes #17584 (auto-closed by the stale-PR bot after ~1 month of inactivity while awaiting staging validation). Same 5 commits, rebased on current
main. No content changes since the last review.The profiler was creating a fresh
ddog_prof_ProfileExporteron every upload cycle (~60s) and dropping it immediately after. Each creation allocates a tokio runtime, TLS connector and HTTP client on the Rust side (~10-15 native allocations per cycle) and churns memory. This PR caches the exporter onProfilerState, reuses it across cycles, and drops it inprefork()on the parent side (while tokio threads are still alive) and incleanup().The exporter is a natural fit for reuse: endpoint (URL + timeout) and static identity tags (env, service, version, runtime, runtime_id, runtime_version, profiler_version, process_id, language) do not change during a process's lifetime. Per-upload user tags can change (manual
Profiler()usage — e.g. Delancie Workers setting a per-task tag), so they are not baked into the exporter; they ride each send viaoptional_additional_tagsinstead.Changes
perf(profiling): reuse profile exporter across upload cycles— cachesddog_prof_ProfileExporteronProfilerState; drops inprefork()(parent side, underupload_lock) andcleanup().Uploaderno longer owns the exporter; reaches it viaProfilerState::get().exporter.refactor(profiling): pass user tags per-send, drop "cached" qualifier— addresses Thomas's user-tag-mutation concern; renamescached_exporter→exporterthroughout.test(profiling): verify user tags differ per upload + restore family/language distinction— addstag_rotation_program.pydriver andtest_per_upload_tags.pymock-agent test (three cycles, three phases; asserts each cycle carries its own tag AND no stale phase leaks into later cycles). Also restoresg_family_namealongsideg_language_nameas distinct constants (they coincide here but are distinct concepts — eBPF profiler can be family=native sampling python).test(profiling): drop nonexistent stop_on_exit kwarg in tag rotation driver—Profiler.start()takes no args in currentmain.chore(profiling): flag UploaderBuilder->ProfilerState hidden dep for follow-up—AIDEV-NOTEatUploaderBuilder::build()documenting that the singleton reach-in is a pre-existing pattern (all 13set_*methods do it) and pointing at a follow-up refactor (decouple via explicit config struct).Testing
tests/profiling/test_per_upload_tags.py::test_user_tags_change_between_uploadspassed (Python 3.12). All threephase:<value>tags land in the right upload body; no cross-cycle leakage.uploaded cycle=N phase=…lines and no upload errors.copy_fast_copy_metadata_from) which co-exists cleanly.Risks
prefork()on the parent side underupload_lock. Both parent and child re-create it lazily on the next upload viaUploaderBuilder::build → ensure_exporter. This is required because the tokio worker threads owned by the Rust runtime do not survive fork; dropping post-fork in the child could deadlock or touch dead-thread state.optional_additional_tags. ManualProfiler()users mutatingddup.config(tags=…)between cycles will now see their new tags reflected on the next upload (was undefined/broken before). Verified bytest_per_upload_tags.py.Follow-up
AIDEV-NOTEatuploader_builder.cpp::UploaderBuilder::build()flags the pre-existingProfilerState::get()reach-in pattern. Follow-up ticket (to be filed): thread an explicit config through theUploaderBuilderAPI to decouple it from theProfilerStatesingleton.