fix(libdd-data-pipeline): cargo clippy fix with all lints#2013
Draft
Eldolfin wants to merge 13 commits into
Draft
fix(libdd-data-pipeline): cargo clippy fix with all lints#2013Eldolfin wants to merge 13 commits into
Eldolfin wants to merge 13 commits into
Conversation
…ebug, items_after_statements, match_wildcard, hidden_lifetime, from_iter, similar_names
…dant_closure, lifetime params, significant_drop_tightening, match_same_arms
…dless_pass_by_value, trivially_copy_pass_by_ref, unused_self, struct_field_names
…ix trivially_copy, needless_pass_by_value, cast_possible_truncation
…eded by workspace lints
Contributor
📚 Documentation Check Results📦
|
|
Contributor
Clippy Allow Annotation ReportComparing clippy allow annotations between branches:
Summary by Rule
Annotation Counts by File
Annotation Stats by Crate
About This ReportThis report tracks Clippy allow annotations for specific rules, showing how they've changed in this PR. Decreasing the number of these annotations generally improves code quality. |
Contributor
🔒 Cargo Deny Results📦
|
…, similar_names rename
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2013 +/- ##
==========================================
- Coverage 72.68% 72.68% -0.01%
==========================================
Files 452 453 +1
Lines 74930 74932 +2
==========================================
- Hits 54463 54462 -1
- Misses 20467 20470 +3
🚀 New features to boost your workflow:
|
…pecific to 1.84.1, fixed in 1.85.0
Contributor
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
|
these appear only in very specific configurations I can't explain...
yannham
reviewed
May 21, 2026
Contributor
yannham
left a comment
yannham
left a comment
There was a problem hiding this comment.
Sorry, I started reviewing before it was switched back to draft, sor so I think. In general I feel there are some renaming business that is unrelated to clippy (unless there's a lint around variable names, but that sounds... surprising?) that could have been a separate (it's not useful to split it, just mentioning it for future reference)
| /// If either the agent state hash or container tags hash is different from the current one: | ||
| /// - Return a `FetchInfoStatus::NewState` of the info struct | ||
| /// - Else return `FetchInfoStatus::SameState` | ||
| #[allow( |
Contributor
There was a problem hiding this comment.
If this is a clippy bug, I wonder if we shouldn't just disable it for the whole file and call it a day... that will be less place to update when we bump to 1.87, and this will happen soon ™️
| // sleep instead of `tokio::time::timeout`, which requires a tokio reactor | ||
| // (not available on wasm where we run on the JS event loop). | ||
| let res = tokio::select! { | ||
| let response = tokio::select! { |
Contributor
There was a problem hiding this comment.
Is it really clippy or unrelated claude changes ? 😛
| /// Leaf crates pin it to a concrete type. | ||
| #[derive(Debug)] | ||
| pub struct AgentInfoFetcher<C: HttpClientCapability + SleepCapability> { | ||
| pub struct AgentInfoFetcher<C: HttpClientCapability + SleepCapability + Sync> { |
Contributor
There was a problem hiding this comment.
@Aaalibaba42 is that really ok to change a sync? This doesn't look like a clippy trivial fix.
| struct Metric { | ||
| name: &'static str, | ||
| metric_type: MetricType, | ||
| kind: MetricType, |
Contributor
There was a problem hiding this comment.
Is this really clippy-related?
| /// another task to send stats when a time bucket expire. When this feature is enabled the | ||
| /// TraceExporter drops all spans that may not be sampled by the agent. | ||
| /// `TraceExporter` drops all spans that may not be sampled by the agent. | ||
| #[allow(missing_docs)] |
Contributor
There was a problem hiding this comment.
Is this missing_docs still relevant (unrelated to the PR, but looks odd)
|
|
||
| #[allow( | ||
| clippy::significant_drop_tightening, | ||
| reason = "guard must be held across the condvar wait; early drop would be incorrect" |
Contributor
There was a problem hiding this comment.
I don't really understand what's at play here. Which guard are we talking about? What line doesn't compile if we remove the allow?
| } | ||
| } | ||
|
|
||
| #[allow( |
Contributor
There was a problem hiding this comment.
This looks fishy, since this function is just a thin wrapper. Isn't rather self.tx.wait_shutdown_done that should have the lint instead?
Comment on lines
+450
to
+455
| let (state, res) = self | ||
| .waiter | ||
| .sender_notifier | ||
| .wait_timeout_while(state, timeout, cond) | ||
| .map_err(|_| TraceBufferError::MutexPoisoned)?; | ||
| drop(state); |
Contributor
There was a problem hiding this comment.
Matter of taste, but I found scope to be better than explicit drop to visualize the lifetime of objects when applicable.
Suggested change
| let (state, res) = self | |
| .waiter | |
| .sender_notifier | |
| .wait_timeout_while(state, timeout, cond) | |
| .map_err(|_| TraceBufferError::MutexPoisoned)?; | |
| drop(state); | |
| let res = { | |
| let (_state, res) = self | |
| .waiter | |
| .sender_notifier | |
| .wait_timeout_while(state, timeout, cond) | |
| .map_err(|_| TraceBufferError::MutexPoisoned)?; | |
| res | |
| }; |
| } | ||
|
|
||
| impl<T> Sender<T> { | ||
| #[allow( |
Contributor
There was a problem hiding this comment.
Which guard and which wait? Again, I find the justification hard to understand
| /// - The first element is the metric to emit | ||
| /// - The second element is an optional tag value for error classification | ||
| #[allow( | ||
| clippy::cast_possible_wrap, |
Contributor
There was a problem hiding this comment.
This isn't needed anymore, since you use u64::try_from?
Contributor
Author
|
Thanks for the review, opening the PR without draft at first was a mistake on my part but I appreciate the early comments ! From what we've discussed IRL the plan would be to be less aggressive in the enabled lints at first to reduce the noise (false positives and refactors without value): |
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
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?
Applies the restrictive workspace Clippy/Rust lint config and opts in
libdd-data-pipeline, fixing the resulting lints without changing behavior.Motivation
Tighten lint coverage incrementally, crate by crate. APMSP-3056
Additional Notes
How to test the change?
CI passing is sufficient validation.