Fix akka-http instrumentation. #899
Conversation
dpratt
force-pushed
the
fix-akka-http
branch
2 times, most recently
from
2ac4876 to
cc00782
Compare
Remove compiled Scala artifacts from the actual instrumentation. Scala is not binary compatible across major versions, and having AkkaHttpClientTransformFlow.scala be in the artifact causes problems when using anything but Scala 2.11. Having the AkkaHttpClientTransformFlow implementation be in pure java utilizing the Akka-stream Java DSL ensures that this will work across any given Scala major version.
|
I'm not entirely sure why the unit tests and the muzzle plugin are failing - it's almost like the test HTTP server isn't being started. Can somebody on the maintainer team point me in the right direction? |
By definition, a Flow generated by a SuperPool does not respect ordering of requests and responses, and in fact will typically only rarely actually behave in the fashion that the instrumentation expects. The previous implementation would start a span for a given request before submitting it as input to the flow, and close the span with whatever response is next emitted by the flow. This request will rarely (if ever) be the actual response for the request that started the span. For more info, see the official docs at https://doc.akka.io/docs/akka-http/current/client-side/host-level.html#configuring-a-host-connection-pool Additionally, compiling this instumentation against scala 2.11, and only scala 2.11 can (and does) cause significant problems at runtime due to the fact that Scala is explicitly not binary compatible across major versions.
|
After more investigation, I'm convinced that it's due to the SuperPool instrumentation. In fact, it's likely that this instrumentation never really worked in the first place, since SuperPools explicitly do not respect ordering of requests and responses. This PR kills two birds with one stone - it fixes some Scala binary compatibility versions w.r.t. the instrumentation, and removes the non-functional instrumentation that was likely generating nonsense/incorrect spans for anybody that was using it. For more info on SuperPool ordering, see https://doc.akka.io/docs/akka-http/current/client-side/host-level.html#configuring-a-host-connection-pool |
|
As near as I can tell, this PR is ready to be merged. I'd love it if you guys could get this in before the next release - my org really wants to upgrade to Scala 2.13 and this is the last thing blocking us. I suspect we're not the only people waiting on this as well. |
|
Could you please clarify why why these renames are needed? IIRC those java classes were put into scala directory just to make compiler/order happy - there were implemented in Java. I'm actually surprised it builds and passes tests with this change. Ass far as SuperPool goes - as far as I can see we had tests for it that looked legit. Is there anything that is wrong with the test that hides problem with the instrumentation? |
|
I suspect that the tests pass by accident, in that a SuperPool is exactly that, a pool of connections. When the flow is supplied with concurrent requests, they will likely be dispatched to different underlying connections, and the responses will be emitted by the flow in any ordering. The current instrumentation makes the incorrect assumption that they will be emitted in the order submitted, which is incorrect. This will cause responses to close spans completely unrelated to their actual request. Essentially, the flow does not respect ordering - if you give it a list of requests [req1, req2, req3, req4] the ordering of the responses is undefined. If they come back as [resp2, resp4, resp1, resp3] then the span timing (and other metadata attached to the span at close time such as the response code, or any error) will be wildly incorrect and the traces will be nonsensical. This is documented at the link I attached, and expected behavior.
Additionally, the fact that the previous implementation worked with both Scala 2.11 and 2.12 was essentially an accident. It is compiled only against 2.11, and Scala has an explicit binary incompatibility across versions. Using artifacts compiled with mismatched versions has undefined or potentially even silently incorrect behavior, and is not supported. If you want the agent to bundle compiled Scala code to be used at runtime, you’ll likely need to have multiple versions of the instrumentation jar compiled against every supported Scala version, and then have the agent pick the right one at runtime.
Renaming the directory was just done as an esthetic cleanup since there’s no longer any actual Scala code in there.
… On Jun 25, 2019, at 8:56 AM, Nikolay Martynov ***@***.***> wrote:
@dpratt
Could you please clarify why why these renames are needed? IIRC those java classes were put into scala directory just to make compiler/order happy - there were implemented in Java. I'm actually surprised it builds and passes tests with this change.
Ass far as SuperPool goes - as far as I can see we had tests for it that looked legit. Is there anything that is wrong with the test that hides problem with the instrumentation?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
There’s an easy way to see the above behavior - make two routes, /a and /b in a test server. Have the implementation of /a pause for 500 milliseconds before returning, and have /b respond immediately. Then, submit requests to the SuperPool flow for /a and /b. You’ll see that the responses are emitted from /b and then /a, which causes the span for the /a request to be closed by the response for /b, and the /b span to be closed by the response for /a. The implementation as it stands today does not work. On top of that, the fact that the compiled version of the instrumentation is linked against Scala 2.11 means that it is only guaranteed to work against Scala 2.11 at runtime. It’s a happy accident that linking it against Scala 2.12 doesn’t throw an exception, but there’s no guarantee that will actually work in the future. Similarly, attempting to use this against Scala 2.13 does not work - it blows up at instrumentation time. |
|
@dpratt I just wanted to clarify: there are essentially three changes:
I.e. there are no changes to specifically support new akka version, old code still works after some renames - is this correct? |
|
That’s precisely it. |
|
Don't want to be a bother - are there plans to merge this in for the next release? |
|
Hi @dpratt first of all thanks a lot for the PR. Let me try to get sometime today or tomorrow for a final review and then we can merge. |
|
Any chance of this making 0.31.0? |
There was a problem hiding this comment.
@dpratt Sorry for the delay. Thanks so much for your contribution. We really appreciate it when domain experts help us fix our instrumentation.
|
I'm using elastic4s for Elasticsearch from Scala with its Akka HTTP client. I was wondering why I couldn't see the HTTP requests, but I'm thinking it's due to lack of support for SuperPool, since I see that being used in elastic4s. Sorry this is a bit off topic, but it seems like people here are the most familiar with Akka HTTP, so I just wanted to see if this is something that would make sense for me to open a feature request for and if that's what's missing for logging calls to the elastic4s code, or if there's some reason that just wouldn't be a reasonable request. |
|
@wuservices would you mind posting about this as a separate issue or via support. |
Remove compiled Scala artifacts from the actual instrumentation. Scala
is not binary compatible across major versions, and having
AkkaHttpClientTransformFlow.scala be in the artifact causes problems
when using anything but Scala 2.11.
Having the AkkaHttpClientTransformFlow implementation be in pure java
utilizing the Akka-stream Java DSL ensures that this will work across
any given Scala major version.