-
Notifications
You must be signed in to change notification settings - Fork 597
feat: Add #[must_use] to AsyncInstrumentBuilder
#3246
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
Conversation
This prevents code like the following from compiling:
```rust
meter
.i64_observable_up_down_counter("my_counter")
.with_callback(|observer| {
observer.observe(1, &[]);
});
```
This code is missing the `.build()` at the end. Before this commit, it would
compile, but the callback would never be called. After this commit, it will
cause a mostly helpful compile error:
```
warning: unused `AsyncInstrumentBuilder` that must be used
--> examples/metrics-basic/src/main.rs:71:5
|
71 | / meter
72 | | .i64_observable_up_down_counter("my_counter")
73 | | .with_callback(|observer| {
74 | | observer.observe(1, &[]);
75 | | });
| |__________^
|
= note: AsyncInstrumentBuilder must be built to receive callbacks.
= note: `#[warn(unused_must_use)]` on by default
help: use `let _ = ...` to ignore the resulting value
|
71 | let _ = meter
| +++++++
```
Most builders need to be built to be useful. Normal metrics are built and then
observations are reported to them, where a failure to call `build()` would be
discovered at compile-time. However, observable metrics (async instruments)
don't have that natural use point, as the result of building an
`AsyncInstrumentBuilder` isn't typically needed. This makes
`AsyncInstrumentBuilder` especially error-prone without the `#[must_use]`
declaration.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3246 +/- ##
=====================================
Coverage 80.8% 80.8%
=====================================
Files 129 129
Lines 23199 23199
=====================================
Hits 18749 18749
Misses 4450 4450 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Even though this breaks only wrong code, it still changes user-visible behavior. Better to add a CHANGELOG.
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.
Pull Request Overview
This PR adds the #[must_use] attribute to AsyncInstrumentBuilder to prevent silent failures when developers forget to call .build() on observable metric builders. This is particularly important for async instruments because their results are typically not stored after building (unlike sync instruments), making it easy to accidentally omit the .build() call, which would cause callbacks to never be invoked.
- Added
#[must_use]attribute with helpful message toAsyncInstrumentBuilderstruct
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I added a CHANGELOG entry. My editor also removed some trailing whitespace in the file, sorry. I've left that in since it's technically an improvement, but let me know if you'd prefer me to revert that. |
opentelemetry/CHANGELOG.md
Outdated
| - **Breaking** Removed the following public fields and methods from the `SpanBuilder` [#3227][3227]: | ||
| - `trace_id`, `span_id`, `end_time`, `status`, `sampling_result` | ||
| - `with_trace_id`, `with_span_id`, `with_end_time`, `with_status`, `with_sampling_result` | ||
| - **Breaking** Added `#[must_use]` to `opentelemetry::metrics::AsyncInstrumentBuilder`, since unbuilt async instruments will not have their callbacks called. This may cause compile errors for buggy or suspicious 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.
https://doc.rust-lang.org/cargo/reference/semver.html#new-lints
We cannot make breaking changes, but this should be acceptable based on rust guidelines. I suggest to avoid "breaking" work in changelog to avoid scaring users.
My suggestion:
- Added
#[must_use]attribute toopentelemetry::metrics::AsyncInstrumentBuilderto add compile time warning when.build()is not called on observable instrument builders, preventing silent failures where callbacks are never registered and metrics are never reported.
cijothomas
left a comment
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.
Change itself is good (thanks for this nice improvement!).
Requesting change to make a better changelog and message (suggestions given as comments already).
Co-authored-by: Cijo Thomas <cithomas@microsoft.com>
|
Ok, @cijothomas rewrote the entire patch 😂, updated. |
@ongardie-atomix If you have interest/bandwidth, we could add a CI check to prevent such things from ever getting checked in. Example in other OTel repos: https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/.github/workflows/sanitycheck.yml#L34 |
|
TIL |
This prevents code like the following from compiling:
meter .i64_observable_up_down_counter("my_counter") .with_callback(|observer| { observer.observe(1, &[]); });This code is missing the
.build()at the end. Before this commit, it would compile, but the callback would never be called. After this commit, it will cause a mostly helpful compile error:Most builders need to be built to be useful. Normal metrics are built and then observations are reported to them, where a failure to call
build()would be discovered at compile-time. However, observable metrics (async instruments) don't have that natural use point, as the result of building anAsyncInstrumentBuilderisn't typically needed. This makesAsyncInstrumentBuilderespecially error-prone without the#[must_use]declaration.Fixes #
Design discussion issue (if applicable) #
Changes
Please provide a brief description of the changes here.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes