-
Notifications
You must be signed in to change notification settings - Fork 901
Promote getAll to TextMapGetter stable API #7267
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7267 +/- ##
============================================
- Coverage 89.92% 89.63% -0.29%
- Complexity 6721 6860 +139
============================================
Files 765 780 +15
Lines 20277 20718 +441
Branches 1985 2020 +35
============================================
+ Hits 18234 18571 +337
- Misses 1448 1510 +62
- Partials 595 637 +42 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
context/src/main/java/io/opentelemetry/context/propagation/TextMapGetter.java
Outdated
Show resolved
Hide resolved
* @return all values for a given {@code key} in order, or returns an empty list. Default method | ||
* wraps {@code get()} as an {@link Iterator}. | ||
*/ | ||
default Iterator<String> getAll(@Nullable C carrier, String key) { |
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.
I'm late to reviewing this, but it was brought to my attention due to micrometer-metrics/tracing#1032. Iterator
as a return type here is not typical in Java APIs. Iterable
would be idiomatic if going for the lowest level of abstraction. An Iterator
is a stateful iteration of an Iterable
. In theory, the Iterator
returned may not be at the first element of the Iterable
. I'm not sure what the maintainers want to do given this has been released, but in my opinion, the API should not be left using Iterator
.
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.
I see there was previous discussion on this in #6852 (comment), with the motivation being to avoid copying from the Enumeration that Servlet spec returns for headers. I think that could be achieved even with the API using Iterable
with one more layer compared to what's there in the Java instrumentation now with EnumerationUtil, like in this Stackoverflow answer. That would make the API nicer for implementations that don't need to deal with the Jakarta Servlet API returning an Enumeration. I'd also point out the keys
method in this same interface returns Iterable<String>
for the header names, and the Jakarta Servlet implementation in the instrumentation repo is copying to a list.
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.
I understand the feedback here - but yeah unfortunately it's too late given this is now stable, it will be very difficult to change.
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.
Thanks for the response.
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.
Please open a new issue to discuss. Hard to stay on top of comments of closed PRs
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.
You could add something like
default Iterator<String> getAll(@Nullable C carrier, String key) {
return getAllValues(carrier, key).iterator();
}
default Iterable<String> getAllValues(@Nullable C carrier, String key) {
String first = get(carrier, key);
if (first == null) {
return Collections.emptyList();
}
return Collections.singleton(first);
}
and deprecate the old method if you so wish.
Operation is now stable in the spec: open-telemetry/opentelemetry-specification#4472