Harmonize @Nullable annotations in okhttp instrumentation - #7375

Taking into account the if condition on the following line, I believe the urlMapper can remain untouched. AFAICT there are no unchecked usages of OkhttpContext.getUrlMapper in the micrometer codebase, so it should be safe to keep the public API in this case.

You are right (this is the only place when it is called). I synchronized it with BiFunction because leaving them different might make things worse for the users when they pass them to the ctor of OkHttpContext.

OkHttpContext(
    Function<@Nullable Request, String> urlMapper,
    ...,
    Iterable<BiFunction<@Nullable Request, @Nullable Response, KeyValue>> contextSpecificTags,
    ...)

I found it confusing to leave Request non-null for the Function but nullable on BiFunction but not sure which one is better for the future (I want to properly fix this).

I went through the codebase this morning with a fresh mind, so if I understand correctly the whole rabbit hole of nullability boils down to the fact that the public API OkHttpContext does not override the nullable SenderContext.getCarrier

I understand that changing the context is not really possible without widely affecting public API, but I believe this doesn't apply to OkHttpMetricsEventListener, which is completely isolated from that context (and pretty isolated in terms of public API, too). In this class, I think we can fully determine nullability and only need <_, @Nullable Response, _>:

• The only entry-point is callStart. Here, Request is guaranteed not null, Response is guaranteed null
• Other lifecycle callbacks do not add new entries to the shared callState map, and do not alter any entry's Request. So, its still guaranteed not null
• responseHeadersEnd potentially updates the Response

Since CallState is private API, we know these are all relevant places, so we should be fine to drop the @Nullable annotation from CallState.Request

I understand that changing the context is not really possible without widely affecting public API,

I think I might be able to fix things by just breaking this:

    public void setState(OkHttpObservationInterceptor.CallState state) {
        this.state = state;
    }

    public OkHttpObservationInterceptor.@Nullable CallState getState() {
        return state;
    }

These are public methods but CallState is package-private so users are not able to use it anyways. (Though I don't like the parameters of the OkHttpContext either, they should be on the convention I think and that's really a breaking change.)

I believe this doesn't apply to OkHttpMetricsEventListener, which is completely isolated from that context (and pretty isolated in terms of public API, too). In this class, I think we can fully determine nullability and only need <_, @nullable Response, _>

I applied the same thought-process here as in #7375 (comment), to keep things consistent(ly wrong).

These are public methods but CallState is package-private so users are not able to use it anyways.

I wonder if there is some static analysis or ErrorProne capability we could use to prevent adding such broken public API to begin with (public API that requires non-public types). I'm fine breaking that in 1.17.0-RC1 since external users can't use the API anyway, right?

They can do context.setState(null) and/or Object obj = context.getState() but neither makes a lot of sense.
I haven't seen any errorprone check for this: https://www.jetbrains.com/help/inspectopedia/ClassEscapesDefinedScope.html#locating-this-inspection
Maybe we should open an issue and/or check if we can use spotbugs or pmd for this.