Make spanKindExtractor configurable in Ktor instrumentations

[orangain]

This PR solves #8139. Now spanKindExtractor is configurable in Ktor instrumentations.

This make it possible to customize the SpanKind depending on the request as following:

    install(KtorServerTracing) {
        setOpenTelemetry(openTelemetry)
        setSpanKindExtractor {
            SpanKindExtractor { req ->
                if (req.uri.startsWith("/from-pubsub/")) {
                    SpanKind.CONSUMER
                } else {
                    SpanKind.SERVER
                }
            }
        }
    }

[orangain]

I am not sure if it is ok to add such a public method to the InstrumenterBuilder, but I could not think of a better way to make SpanKindExtractor configurable.

[mateuszrzeszutek]

Yeah, it is necessary to add a new method here (I kinda forgot about it earlier, when we were talking in GH discussions).

Since this is a stable public API, we need to make sure we come up with a good name. I'll list a few ideas I came up with here:

• buildIncomingInstrumenter/buildOutgoingInstrumenter (pretty much what this PR proposes)
• buildContextExtractingInstrumenter/buildContextInjectingInstrumenter
• buildUpstreamInstrumenter/buildDownstreamInstrumeter
• buildPropagatingToUpstreamInstrumenter/buildPropagatingToDownstreamInstrumeter
• buildPropagatingInstrumenter for both

@open-telemetry/java-instrumentation-maintainers WDYT?

[laurit]

Naming is hard. Perhaps we should adopt a @Incubating or @Experimental annotations so such changes could be introduced more easily.
I think it is possible to implement this feature without introducing new api. You could use buildInstrumenter(spanKindExtractor) and do the context extraction manually before instrumenter.start() similarly to like it is done in PropagatingFromUpstreamInstrumenter.

[orangain]

@laurit Thank you for your suggestion. I tried implementing this without introducing a new API.
orangain@daec120

While it is indeed possible to implement, I have a few concerns.

• Doing context propagation within library instrumentation seems to be a violation of responsibilities. Wouldn't it be confusing if only Ktor instrumentation does context propagation on its own, while Instrumenter does it in other library instrumentations?
• ContextPropagationDebug exists in the internal package, is it acceptable to use it in Ktor instrumentation? If not allowed, there is a concern that Ktor users will not be able to debug context propagation.

[laurit]

@orangain While letting Instrumenter handle context propagation is preferable there are instrumentations that do it manually for a variety of reasons. This was just a suggestion on how to move this forward faster, if you so wish you can ignore it. You could also start with manual propagation and when the methods you need are added to Instrumenter you can start using them.
Using internal classes is fine everywhere in this repository. Marking classes internal is more for informing users outside this repository that these classes could change without prior notice. I think that in this case not using ContextPropagationDebug would also be fine.

[mateuszrzeszutek]

Hey @orangain I opened #8392 to unblock you PR -- I'll let you know when it's merged so that you can rebase on top of it.

[trask]

hey @orangain, #8392 is merged now

[orangain]

@mateuszrzeszutek @trask @laurit Sorry for the late response. Thank you for unblocking my PR! I've rebased the PR to use InstrumenterUtil.buildUpstreamInstrumenter.

[orangain]

This method is not used at this time, but I added it to make it symmetrical with the buildIncomingInstrumenter.

[orangain]

I have not written any tests for this method, as there were no unit tests for the existing setStatusExtractor or addAttributeExtractor, is that ok?

[mateuszrzeszutek]

We have some integration tests that exercise the entire instrumentation module.
You could perhaps use the AbstractHttpServerUsingTest and HttpServerInstrumentationExtension directly and set up a simple HTTP server with one endpoint, just to verify that the span it emits has the span kind you want.

[orangain]

Thank you for your detailed explanation, I added test cases in 959ee02.

[mateuszrzeszutek]

Thanks @orangain !
LGTM 👍