TracerFlare: save original log level as indication we are currently collecting #1272
Conversation
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
Aaalibaba42
force-pushed
the
jwiriath/add-debug-levels-in-tracerflare-cc
branch
from
5314358 to
88afef4
Compare
BenchmarksComparisonBenchmark execution time: 2025-10-24 14:05:36 Comparing candidate commit d10fff8 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 55 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
Group 14
Group 15
Group 16
Group 17
BaselineOmitted due to size. |
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
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1272 +/- ##
==========================================
+ Coverage 71.82% 71.83% +0.01%
==========================================
Files 368 368
Lines 57967 57957 -10
==========================================
+ Hits 41633 41635 +2
+ Misses 16334 16322 -12
🚀 New features to boost your workflow:
|
Aaalibaba42
commented
Oct 21, 2025
Aaalibaba42
commented
Oct 21, 2025
Aaalibaba42
force-pushed
the
jwiriath/add-debug-levels-in-tracerflare-cc
branch
4 times, most recently
from
49d542a to
512c624
Compare
…ep current log level
Aaalibaba42
force-pushed
the
jwiriath/add-debug-levels-in-tracerflare-cc
branch
from
512c624 to
6cdb6e4
Compare
Aaalibaba42
force-pushed
the
jwiriath/add-debug-levels-in-tracerflare-cc
branch
from
0b8b3db to
67e886f
Compare
datadog-tracer-flare/src/lib.rs
Outdated
| } else if Ok(ReturnAction::None) != action { | ||
| // If action is Send, Unset or an error, we need to stop collecting | ||
| self.collecting.store(false, Ordering::Relaxed); | ||
| *self.current_log_level.lock_or_panic() = None; |
There was a problem hiding this comment.
If you remove the current_log_level is the case of Send, you'll fall into the "Log level is not set" every time the zip_and_send function is called. With collecting only it was not a problem but with the log level you should split this case in two.
There was a problem hiding this comment.
I see what you mean, then maybe just putting getter/setters for the integration would work
There was a problem hiding this comment.
Okay I think the best way is indeed to just have both log_levels fields public, and have the collecting bool as before. That way we don't change anything to the ReturnAction we return, and the integrater can manage these to make it work out.
datadog-tracer-flare/src/lib.rs
Outdated
| *tracer_flare.current_log_level.lock_or_panic() = Some(log_level); | ||
| } else if let ReturnAction::Send(_) = state { | ||
| tracer_flare.collecting.store(false, Ordering::Relaxed); | ||
| *tracer_flare.current_log_level.lock_or_panic() = None; |
There was a problem hiding this comment.
Same here you should not remove the current_log_level. It should be kept until the Send is done and then you can put it back as None.
There was a problem hiding this comment.
Yup good point that's my bad
datadog-tracer-flare/src/zip.rs
Outdated
| .current_log_level | ||
| .lock_or_panic() | ||
| .ok_or(FlareError::ZipError(String::from( | ||
| "Trying to send the flare when log_level is not set", |
There was a problem hiding this comment.
The case when you receive only an AGENT_TASK and thus only have to do a Send action is possible. In that case we still want to send the flare. So we either have the choice to use the tracer actual log_level or we do as Python and Java and putting by default the log_level which is Debug. I think putting Debug would be the best option but it's also more complicated to handle in a design way.
There was a problem hiding this comment.
I don't think it complicates anything. Maybe we can make the field public so that the integrater can set it if it is not set correctly, but it should make things automagic most of the time
…re for the integrater only
Aaalibaba42
force-pushed
the
jwiriath/add-debug-levels-in-tracerflare-cc
branch
2 times, most recently
from
60ad4cc to
476b25d
Compare
Aaalibaba42
force-pushed
the
jwiriath/add-debug-levels-in-tracerflare-cc
branch
from
476b25d to
5ddde0f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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?
Change the collecting boolean field to an Option (inside a Mutex for reasons, but that's beside the point) in the TracerFlareManager. Also adding a
original_log_levelfield that is managed by the integrater code.Motivation
Basically, the integration will have to restore original log level after the Flare is sent, which is impractical because it therefore needs to be saved in a "global" state somewhere. But the TracerFlareManager already has this global scope requirement, thus storing the original log level in it makes sens. The change of the
collectingfield is just about simplifying the API, since now functions likezip_and_sendtake less arguments and stuff.Additional Notes
Anticipating what's going through FFIs once it's implemented could be interesting.
How to test the change?
cargo test(it's still the design phase, it's not integrated anywhere).