Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7934 +/- ##
============================================
+ Coverage 90.11% 90.14% +0.02%
- Complexity 7463 7487 +24
============================================
Files 834 835 +1
Lines 22586 22628 +42
Branches 2240 2243 +3
============================================
+ Hits 20353 20397 +44
+ Misses 1532 1528 -4
- Partials 701 703 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| * @deprecated {@code otel_scope_*} attributes are always generated. | ||
| */ | ||
| /** Set if the {@code otel_scope_*} attributes are generated. Default is {@code true}. */ | ||
| @SuppressWarnings("UnusedReturnValue") |
There was a problem hiding this comment.
These suppressions should go on the caller, not this method. If we wanted to inform callers that the response can safely be ignored, we'd use @CanIgnoreReturnValue.
https://errorprone.info/bugpattern/CheckReturnValue
I think you probably copy / pasted this. It must have slipped through the cracks.
There was a problem hiding this comment.
more like I didn't catch it from the AI - added @CanIgnoreReturnValue for the entire class now
There was a problem hiding this comment.
Sorry should have mentioned this in my initial comment, but we actually explicitly ignore the CanIgnoreReturnValueSuggester from error prone. (see here for the original commit)
If we want to add @CanIgnoreReturnValue, I think we'd want to do it holistically. Or at least commit to doing it holistically. WDYT @jkwatson? Is it worth it to go through all the builders and add an annotation?
There was a problem hiding this comment.
I'll remove it from the PR to unblock.
...etheus/src/main/java/io/opentelemetry/exporter/prometheus/PrometheusMetricReaderBuilder.java
Show resolved
Hide resolved
...rometheus/src/test/java/io/opentelemetry/exporter/prometheus/PrometheusMetricReaderTest.java
Outdated
Show resolved
Hide resolved
jack-berg
left a comment
There was a problem hiding this comment.
Couple of nits but looks good. Thanks!
57bbd9c to
9cff6a4
Compare
...ters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/PrometheusHttpServer.java
Outdated
Show resolved
Hide resolved
...ometheus/src/main/java/io/opentelemetry/exporter/prometheus/PrometheusHttpServerBuilder.java
Outdated
Show resolved
Hide resolved
|
@jack-berg please have another look |
Update to latest declarative configuration.
See open-telemetry/opentelemetry-configuration#474 for a clarification in semantics.