Skip to content

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Apr 15, 2025
Merged

Promote getAll to TextMapGetter stable API #7267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
21758f9
Promote getAll to TextMapGetter stable API
jack-berg Apr 11, 2025
b2193a6
Remove outdated comment
jack-berg Apr 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 2 additions & 25 deletions api/all/src/main/java/io/opentelemetry/api/baggage/propagation/W3CBaggagePropagator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
import io.opentelemetry.context.propagation.TextMapGetter;
import io.opentelemetry.context.propagation.TextMapPropagator;
import io.opentelemetry.context.propagation.TextMapSetter;
import io.opentelemetry.context.propagation.internal.ExtendedTextMapGetter;
import java.util.Collection;
import java.util.Iterator;
import java.util.List;
Expand Down Expand Up @@ -97,33 +96,11 @@ public <C> Context extract(Context context, @Nullable C carrier, TextMapGetter<C
return context;
}

if (getter instanceof ExtendedTextMapGetter) {
return extractMulti(context, carrier, (ExtendedTextMapGetter<C>) getter);
}
return extractSingle(context, carrier, getter);
}

private static <C> Context extractSingle(
Context context, @Nullable C carrier, TextMapGetter<C> getter) {
String baggageHeader = getter.get(carrier, FIELD);
if (baggageHeader == null) {
return context;
}
if (baggageHeader.isEmpty()) {
return context;
}

BaggageBuilder baggageBuilder = Baggage.builder();
try {
extractEntries(baggageHeader, baggageBuilder);
} catch (RuntimeException e) {
return context;
}
return context.with(baggageBuilder.build());
return extractMulti(context, carrier, getter);
}

private static <C> Context extractMulti(
Context context, @Nullable C carrier, ExtendedTextMapGetter<C> getter) {
Context context, @Nullable C carrier, TextMapGetter<C> getter) {
Iterator<String> baggageHeaders = getter.getAll(carrier, FIELD);
if (baggageHeaders == null) {
return context;
Expand Down
5 changes: 2 additions & 3 deletions api/all/src/test/java/io/opentelemetry/api/baggage/propagation/W3CBaggagePropagatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
import io.opentelemetry.api.baggage.BaggageEntryMetadata;
import io.opentelemetry.context.Context;
import io.opentelemetry.context.propagation.TextMapGetter;
import io.opentelemetry.context.propagation.internal.ExtendedTextMapGetter;
import java.util.Collections;
import java.util.HashMap;
import java.util.Iterator;
Expand All @@ -40,8 +39,8 @@ public String get(Map<String, String> carrier, String key) {
}
};

private static final ExtendedTextMapGetter<Map<String, List<String>>> multiGetter =
new ExtendedTextMapGetter<Map<String, List<String>>>() {
private static final TextMapGetter<Map<String, List<String>>> multiGetter =
new TextMapGetter<Map<String, List<String>>>() {
@Override
public Iterable<String> keys(Map<String, List<String>> carrier) {
return carrier.keySet();
Expand Down
21 changes: 21 additions & 0 deletions context/src/main/java/io/opentelemetry/context/propagation/TextMapGetter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@

package io.opentelemetry.context.propagation;

import java.util.Collections;
import java.util.Iterator;
import javax.annotation.Nullable;

/**
Expand Down Expand Up @@ -33,4 +35,23 @@ public interface TextMapGetter<C> {
*/
@Nullable
String get(@Nullable C carrier, String key);

/**
* If implemented, returns all values for a given {@code key} in order, or returns an empty list.
*
* <p>The default method returns the first value of the given propagation {@code key} as a
* singleton list, or returns an empty list.
*
* @param carrier carrier of propagation fields, such as an http request.
* @param key the key of the field.
* @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) {
Copy link

@shakuzen shakuzen May 28, 2025

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.

Copy link

@shakuzen shakuzen May 28, 2025

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.

Copy link
Contributor

@jamesmoessis jamesmoessis Jul 15, 2025

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.

Copy link

@shakuzen shakuzen Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the response.

Copy link
Member Author

@jack-berg jack-berg Jul 15, 2025

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

Copy link
Contributor

@laurit laurit Jul 15, 2025

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.

String first = get(carrier, key);
if (first == null) {
return Collections.emptyIterator();
}
return Collections.singleton(first).iterator();
}
}
31 changes: 4 additions & 27 deletions ...xt/src/main/java/io/opentelemetry/context/propagation/internal/ExtendedTextMapGetter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,39 +6,16 @@
package io.opentelemetry.context.propagation.internal;

import io.opentelemetry.context.propagation.TextMapGetter;
import java.util.Collections;
import java.util.Iterator;
import javax.annotation.Nullable;

/**
* Extends {@link TextMapGetter} to return possibly multiple values for a given key.
* Extended {@link TextMapGetter} with experimental APIs.
*
* <p>This class is internal and experimental. Its APIs are unstable and can change at any time. Its
* APIs (or a version of them) may be promoted to the public stable API in the future, but no
* guarantees are made.
*
* @param <C> carrier of propagation fields, such as an http request.
*/
public interface ExtendedTextMapGetter<C> extends TextMapGetter<C> {
/**
* If implemented, returns all values for a given {@code key} in order, or returns an empty list.
*
* <p>The default method returns the first value of the given propagation {@code key} as a
* singleton list, or returns an empty list.
*
* <p>This class is internal and is hence not for public use. Its APIs are unstable and can change
* at any time.
*
* @param carrier carrier of propagation fields, such as an http request.
* @param key the key of the field.
* @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) {
String first = get(carrier, key);
if (first == null) {
return Collections.emptyIterator();
}
return Collections.singleton(first).iterator();
}

// keep this class even if it is empty, since experimental methods may be added in the future.

}
9 changes: 5 additions & 4 deletions ...n/internal/ExtendedTextMapGetterTest.java → ...ontext/propagation/TextMapGetterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,22 @@
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.context.propagation.internal;
package io.opentelemetry.context.propagation;

import static org.assertj.core.api.AssertionsForClassTypes.assertThat;

import com.google.common.collect.ImmutableList;
import io.opentelemetry.context.propagation.internal.ExtendedTextMapGetter;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Iterator;
import java.util.List;
import javax.annotation.Nullable;
import org.junit.jupiter.api.Test;

class ExtendedTextMapGetterTest {
class TextMapGetterTest {

final ExtendedTextMapGetter<Void> nullGet =
final TextMapGetter<Void> nullGet =
new ExtendedTextMapGetter<Void>() {
@Override
public Iterable<String> keys(Void carrier) {
Expand All @@ -31,7 +32,7 @@ public String get(@Nullable Void carrier, String key) {
}
};

final ExtendedTextMapGetter<Void> nonNullGet =
final TextMapGetter<Void> nonNullGet =
new ExtendedTextMapGetter<Void>() {
@Override
public Iterable<String> keys(Void carrier) {
Expand Down
5 changes: 4 additions & 1 deletion docs/apidiffs/current_vs_latest/opentelemetry-context.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,5 @@
Comparing source compatibility of opentelemetry-context-1.50.0-SNAPSHOT.jar against opentelemetry-context-1.49.0.jar
No changes.
*** MODIFIED INTERFACE: PUBLIC ABSTRACT io.opentelemetry.context.propagation.TextMapGetter (not serializable)
=== CLASS FILE FORMAT VERSION: 52.0 <- 52.0
GENERIC TEMPLATES: === C:java.lang.Object
+++ NEW METHOD: PUBLIC(+) java.util.Iterator<java.lang.String> getAll(java.lang.Object, java.lang.String)
Loading