fix(tracer-flare): fix design of TracerFlareManager #1186
+181
−144
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
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1186 +/- ##
==========================================
- Coverage 71.85% 71.76% -0.09%
==========================================
Files 356 356
Lines 56685 56680 -5
==========================================
- Hits 40731 40679 -52
- Misses 15954 16001 +47
🚀 New features to boost your workflow:
|
ekump
reviewed
Aug 20, 2025
BenchmarksComparisonBenchmark execution time: 2025-10-07 21:12:56 Comparing candidate commit d1b051f in PR branch Found 2 performance improvements and 0 performance regressions! Performance is the same for 51 metrics, 2 unstable metrics. scenario:credit_card/is_card_number/378282246310005
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
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
|
ekump
reviewed
Sep 29, 2025
VianneyRuhlmann
approved these changes
Oct 8, 2025
| /// | ||
| /// Key responsibilities: | ||
| /// - Parsing remote configuration files to determine flare actions (Set log level, Send flare) | ||
| /// - Storing agent task metadata needed for flare transmission |
There was a problem hiding this comment.
Isn't this outdated since you send the task in the action now ?
| /// managing the lifecycle of flare collection and transmission. It operates in two modes: | ||
| /// | ||
| /// - **Basic mode**: Stores agent URL and language configuration for flare operations | ||
| /// - **Remote config mode**: Listens to remote configuration updates to automatically trigger flare |
There was a problem hiding this comment.
nit: I find the name "Remote config mode" a bit confusing as both mode rely on remote config maybe the "no-listener" and "listener" mode is clearer ?
|
|
||
| /// Enum that hold the different log level possible | ||
| #[derive(Debug, PartialEq)] | ||
| /// Enum that holds the different log levels possible |
There was a problem hiding this comment.
You should explicit that we rely on Ord to choose which level to keep and therefore should not change the order of the enum variants.
There was a problem hiding this comment.
+1, we should also have a unit test for the ordering.
| pub enum ReturnAction { | ||
| /// If AGENT_CONFIG received with the right properties. | ||
| Start(LogLevel), | ||
| /// If AGENT_TASK received with the right properties. |
There was a problem hiding this comment.
More than "when" this action is returned you should explain "what" the tracer should do when receiving this action.
| /// * `FlareError(msg)` - If something fail. | ||
| pub fn handle_remote_config_file( | ||
| file: RemoteConfigFile, | ||
| tracer_flare: &mut TracerFlareManager, |
There was a problem hiding this comment.
Why is this not a method of the tracer flare manager ?
| file: RemoteConfigFile, | ||
| tracer_flare: &mut TracerFlareManager, | ||
| ) -> Result<ReturnAction, FlareError> { | ||
| fn check_remote_config_file(file: RemoteConfigFile) -> Result<ReturnAction, FlareError> { |
There was a problem hiding this comment.
nit: I find the names check_remote_config_file and check_remote_config_file a bit confusing maybe this could be parse_remote_config_file or an Implementation of TryFrom<RemoteConfigFile> on ReturnAction
| if let ReturnAction::Set(_) = state { | ||
| tracer_flare.collecting = true; | ||
| } else if let ReturnAction::Send(_) = state { | ||
| tracer_flare.collecting = false; |
There was a problem hiding this comment.
Should Unset also set collecting to false ?
There was a problem hiding this comment.
+1, if you don't set collecting to false on Unset doesn't that mean you can't start any subsequent flares?
Is it possible to cover these scenarios with unit tests? Or do they require integration tests?
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
It would be nice to have a test for priority as it can also be used as reference for other implementations.
ekump
approved these changes
Oct 9, 2025
There was a problem hiding this comment.
Conditional LGTM, providing you address @VianneyRuhlmann's comments / suggestions. In particular setting tracer_flare.collecting = false; on Unset seems important.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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?
Fix the use of the
AGENT_CONFIGproduct in the tracer flare design.Remove the
statefield.Added an argument
log_levelfor thezip_and_sendfunction to let the language tell us what log level is on when creating the flare.We're now returning a list of action to perform from the function that listen the RC.
Motivation
What inspired you to submit this pull request?
Additional Notes
How to test the change?
Describe here in detail how the change can be validated.