-
Notifications
You must be signed in to change notification settings - Fork 13
feat(profiling): upload compression config #1064
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat(profiling): upload compression config #1064
Conversation
BenchmarksComparisonBenchmark execution time: 2025-05-29 18:00:47 Comparing candidate commit 35933df in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 52 metrics, 2 unstable metrics. CandidateCandidate benchmark detailsGroup 1
Group 2
Group 3
Group 4
Group 5
Group 6
Group 7
Group 8
Group 9
Group 10
Group 11
Group 12
Group 13
BaselineOmitted due to size. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1064 +/- ##
==========================================
+ Coverage 70.91% 70.92% +0.01%
==========================================
Files 329 329
Lines 49877 49916 +39
==========================================
+ Hits 35369 35403 +34
- Misses 14508 14513 +5
🚀 New features to boost your workflow:
|
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
|
What kind of env var is this? Did you mean |
I wonder if for this feature it's worth implementing it in a slightly different way. E.g. the current approach of exposing the compression as a flag means:
As an alternative, it would be really cool if libdatadog just looked at This means a bit less control for the libdatadog callers but... again in this case may be worth having the exception? |
Aligning on env vars is a good idea, @felixge proposed a convention (you can refer to internal notebook) |
...
Though do you want libdatadog APIs calling getenv ? Or would you document this as being part of the init process? |
In general no, but I think this particular situation warrants doing that. |
Runtimes need to manage the env var, partly because calling Regarding I can make a parsing function like |
I'm with you -- Ruby also has a config-via-code that usually we try to respect. What I'm suggesting is -- in this case, I don't think it's worth respecting it? Since this is supposed to be for internal testing anyway, so it seems reasonable to say it can only be set via env flag. |
Pull request was converted to draft
Proposal: we have a flag on the profiler object that defaults to |
5ea6e93
to
d301705
Compare
d301705
to
8fa167e
Compare
714ecc0
to
5d54a41
Compare
fc0bfe1
to
68be8ec
Compare
5d54a41
to
9cafb27
Compare
68be8ec
to
623bb6e
Compare
c4c8301
to
49d50fa
Compare
623bb6e
to
3634bc9
Compare
49d50fa
to
9fa834b
Compare
9fa834b
to
35933df
Compare
What does this PR do?
Adds upload compression configuration. For now, the options are Off,
On, and Lz4:
Also adds some conversions between FFI Result types and
std::result::Result
and exposes some more public members, so thatthey can be used in examples.
Motivation
This is being done in preparation for having Zstd as an option, and
possibly the new default.
Additional Notes
We may want
roundtrip_to_pprof
to be configurable on whatcompression algorithm it uses.
None
would speed up tests,but once we have zstd, we may wish to test each one.
How to test the change?
There's nothing to change unless you want to test out the different
compression algorithms. In that case, call
ddog_prof_Profile_set_upload_compression
before uploading.