The instrumentation should work when using only http1 protocol

[jtjeferreira]

This is just a PR to demonstrate a problem that I think I found with the akka-http instrumentation...

AFAICT akka-http with http2 preview support enabled should support both http1/1 and http2. However the instrumentation does not seem to kick in when http1/1 is used...

I will try to explore this in the next few days, but decided to raise the topic before I dig deeper...

[jtjeferreira]

The problem is that this code (https://github.com/akka/akka-http/blob/df3dbfbb8344844fdf4f9a2b3d8aebe3793bfc56/akka-http-core/src/main/scala/akka/http/impl/engine/http2/Http2.scala#L72-L73) is not being fully instrumented.

val http1 = Flow[HttpRequest].mapAsync(settings.pipeliningLimit)(handleUpgradeRequests(handler, settings, log)).join(http.serverLayer(settings, log = log))
val http2 = Http2Blueprint.handleWithStreamIdHeader(settings.http2Settings.maxConcurrentStreams)(handler)(system.dispatcher).join(Http2Blueprint.serverStackTls(settings, log, telemetry, Http().dateHeaderRendering))

Only the http2 flow is instrumented, the http1 flow is only partially instrumented...

[jtjeferreira]

@ivantopo I see you are hacking akka-http, so you might want to have a look here. At the time I was really out of ideas to properly instrument this...

[ivantopo]

Hey @jtjeferreira!

I'll dedicate a bit of time to this today. Let's see what I can do! I do remember long time ago that in order to make it work for HTTP1 AND HTTP2 at the same time we would need to copy/paste a bunch of code in a super ugly way, but maybe things changed. Let's see!

[ivantopo]

It's me again, @jtjeferreira:

Indeed, you got to the right place:

val http1 = Flow[HttpRequest].mapAsync(settings.pipeliningLimit)(handleUpgradeRequests(handler, settings, log)).join(http.serverLayer(settings, log = log))

That definition of http1's flow is what we need to change, but we can't change local variables via instrumentation. The only way we could make it work is by instrumenting the entire akka.http.impl.engine.http2.Http2Ext#bindAndHandleAsync method, swapping it for another implementation that adds our flow wrapper to the http1 flow. In the process we would also need to copy a bunch of other stuff that supports the implementation.

There might be a dirty alternative, though. If we add some context around Http2Ext.bindAndHandleAsync and instrument akka.stream.scaladsl.FlowOps#mapAsync to transform the flow if, and only if called from bingAndHandleAsync then we could do the wrapping there.

I'm going to experiment on that and see where it takes me.

[ivantopo]

Got this to work, but there is a tiny issue: if an HTTP/1 over HTTPS request is sent to an HTTP/2-enabled server, the http.uri tag is reported as http://..... instead of https://..... But this seems to come directly from Akka HTTP's implementation itself so maybe I should just fix the assertions on the tests and move on 🤔

[ivantopo]

Hey @jtjeferreira 👋

I pushed one commit to your branch with the changes and it would be awesome if you can give a try and share your experience. All tests are tied to an older version of Akka HTTP and I'm not 100% sure it is working on newer versions. I'll try to test it in the next couple days too.

[jtjeferreira]

Hi @ivantopo.

Thanks for this. I will try to have a look in the next days...

[ivantopo]

I had time to test this properly this morning and indeed, it works!

These are the requests I sent (each one 4 times):

curl https://127.0.0.1:8080/prior-knowledge -vv --insecure --http2-prior-knowledge
curl https://127.0.0.1:8080/with-upgrade -vv --insecure --http2
curl https://127.0.0.1:8080/plain-old-http1 -vv --insecure --http1.1

And I got traces for all them:

I'm merging this and releasing it with 2.5.1 🎉