Usage of `@Nullable` in the TextMapPropagator seems random

[bogdandrutu]

Example:

Why in some APIs carrier is nullable and in others non-null?

  interface Getter<C> {

    /**
     * Returns all the keys in the given carrier.
     *
     * @param carrier carrier of propagation fields, such as an http request.
     * @since 0.10.0
     */
    Iterable<String> keys(C carrier);  // ** Why not here? **

    /**
     * Returns the first value of the given propagation {@code key} or returns {@code null}.
     *
     * @param carrier carrier of propagation fields, such as an http request.
     * @param key the key of the field.
     * @return the first value of the given propagation {@code key} or returns {@code null}.
     */
    @Nullable
    String get(@Nullable C carrier, String key);  // ** Why here? **
  }

Why is carrier nullable? Can we document if there is a good reason?

  <C> void inject(Context context, @Nullable C carrier, Setter<C> setter);
  <C> Context extract(Context context, @Nullable C carrier, Getter<C> getter);

[jkwatson]

This was introduced at the request of @Oberon00 to support lambda usages. Additional documentation as to 'why' would be good.

[bogdandrutu]

Then I would add that on the Iterable<String> keys(C carrier); as well because that seems forgotten

[bogdandrutu]

Documentation and maybe change the keys API to also support nullable can be done after GA, I think adding the annotation is ABI compatible.

[jkwatson]

Looking at this carefully, the @Nullable annotation on the Getter might not make sense any more, since the Getter is no longer a single-method interface, and hence can't be implemented as a lambda.

I actually think that the decision to make the parameter @Nullable has resulted in a slightly confusing API. It might be easier just to remove them all from these APIs and let the instrumentation implementation that is doing the propagation work out how it will work best for them. If the instrumentation author knows that they will pass in a null carrier then they can deal with the compiler warnings in their specific cases, and leave this API simpler for the 99% use-case.

[bogdandrutu]

@Oberon00 can you please give details if this is still needed?

[Oberon00]

This was very useful for unit tests, etc. where using a lambda to implement the single-method interface worked nicely. Now that it no longer is one, I think we can remove the @Nullable but we should still document to implementers if they actually may throw NullPointerExceptions (I think they must not if we follow the error handling guidelines)

[anuraaga]

If we remove @Nullable from the Getter, we would want to remove it from the propagator too

https://github.com/open-telemetry/opentelemetry-java/blob/main/api/context/src/main/java/io/opentelemetry/context/propagation/TextMapPropagator.java#L108

It becomes a bit complicated though, what happens if some propagators return Context.invalid or inject no-op if the carrier is null while others don't? It seems safe enough but I think losing the annotation does open up losing the semantics of "just pipe the carrier through" we have right now. Having the annotation seems harmless enough, I think it's relatively standard for interfaces to accept input more loosely than the actual implementation. So maybe we can just add Nullable to keys.

Alternatively, if we feel there are so few propagators in the world, we could update all of ours to handle null as no-op so instrumentation doesn't have to worry. It would be a late chance expecting this from any propagators that are out there though.

[hdost]

I see two merged PRs related to this what further would need to be done here?