-
Notifications
You must be signed in to change notification settings - Fork 369
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
Upgrade to libdatadog 0.9.0.1.0 #2302
Conversation
This latest version brings a lot of renames to clean up the last remnants of the library being called libddprof instead of libdatadog. This PR only updates our code to use the new names, and otherwise behavior is unchanged (and all specs are still passing).
Starting with version 0.9, libdatadog takes care of compression for us, so we mustn't compress the payload ourselves to avoid double compression (which the backend will reject -- I've tried).
Libdatadog 0.9 has now been released and is up on rubygems.org. Marking this PR as ready for review :) |
Libdatadog 0.9 reports data using the intake v2.4 format, and we were previously using v1.3. Thus, the integration specs need to be updated to match the new format. One additional change also included here is testing that the file payloads are compressed using LZ4 (which is also a new libdatadog 0.9 feature).
Its only user was the profiler, which no longer needs it.
This gem uses native code and is not compatible with JRuby.
af360a4
to
7cbd3c4
Compare
pprof_data: Datadog::Core::Utils::Compression.gzip(uncompressed_pprof), | ||
pprof_data: uncompressed_pprof.to_s, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the data just left uncompressed here now? I assume it's going to be compressed later down the line, before it's flushed out of the process?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly; it goes from the Profiling::Exporter
to the Profiling::HttpTransport
uncompressed. The lifetime for the pprof data is quite short (until the report is successful), so I don't expect this to have a noticeable impact on application memory.
# Common database-related utility functions. | ||
# Compression/decompression utility functions. | ||
# | ||
# @deprecated This is no longer used by ddtrace and will be removed in 2.0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to point to the new Compression replacement, or is there none?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There isn't any, since libdatadog ships the lz4 compression inside itself and does not (currently) expose it to callers. (Which is why I needed to get the extlz4
gem for the rspec testing)
require 'extlz4' # Lazily required, to avoid trying to load it on JRuby | ||
|
||
expect(LZ4.decode(body.fetch(pprof_file_name))).to eq pprof_data | ||
expect(LZ4.decode(body.fetch(code_provenance_file_name))).to eq code_provenance_data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find to understand where the LZ4 compression is happening after the changes.
I can only assume it now lives in libdatadog, but I can't find any hints outside of this file of any LZ4 compression (or any form of compression) being done, even if by calls to external libdatadog
methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The compression now entirely lives inside libdatadog, and is transparent. When calling ddog_ProfileExporter_send
the byte arrays it receives with the data is compressed and sent to the backend.
I can only assume it now lives in libdatadog, but I can't find any hints outside of this file of any LZ4 compression (or any form of compression) being done
You're right. And in hindsight maybe it's a bit too cryptic on the test.
I'll push a commit to move these tests to a specific it 'reports the payload as lz4-compressed files, that get compressed by libdatadog'
so that this fact is clearly called out and documented in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in c892008
Codecov Report
@@ Coverage Diff @@
## master #2302 +/- ##
==========================================
+ Coverage 97.54% 97.55% +0.01%
==========================================
Files 1076 1076
Lines 56702 56709 +7
==========================================
+ Hits 55309 55322 +13
+ Misses 1393 1387 -6
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
What does this PR do?
Upgrade dd-trace-rb to use the latest libdatadog release (0.9.0.1.0).
This PR also incorporates the upgrade to version 0.8.0.1.0 initially worked on in #2240 but that ended up being blocked by DataDog/libdatadog#47 and thus never merged.
I strongly recommend reviewing this PR commit-by-commit 😄
Expect the following changes:
Profiling::Exporter
, since libdatadog handles compression nowProfiling::HttpTransport
Profiling::HttpTransport
integration tests now expect libdatadog to report in the intake v2.4 format, including automatic lz4 compression of data.Datadog::Core::Utils::Compression
as deprecated for removal, since profiling was its only userMotivation
Keep up with latest improvements and features being added to libdatadog.
Additional Notes
I'm opening this PR:
as a draft because I'm still waiting for the libdatadog 0.9 stable releaseHow to test the change?
The changes in this PR either include test coverage or are already covered by existing tests.