Discussion: Instrumentation hooks allowing timing entire requests

[lf-]

Hi! I'm posting this as part of investigating improving @iand675's hs-opentelemetry library's support for http-client.

If you're not familiar, OpenTelemetry lets people trace requests through distributed systems, with any number of services in them. It allows making spans for things that are happening at some point in time, as well as attaching metadata to them. I've used this technology at work to figure out why things are slow, for extracting database statements, and more.

Currently, the http-client instrumentation for hs-opentelemetry cannot instrument requests without requiring every library using it to import the instrumentation in place of http-client. This effectively means that libraries can't be instrumented, since they would gain a dependency on hs-opentelemetry-instrumentation-http-client.

There are two conceptual things that need to be done to HTTP requests to instrument them:

• add headers from the propagator, to propagate traces to downstream systems
• call an IO action to start and end the timing of the request

As far as I can tell, the current hooks in Manager aren't sufficient to fully instrument requests by merely changing the Manager. For instance, I could creatively use managerWrapException to time starting a request and receiving headers, but it's unclear how I could time the entire httpLbs since in large part, the hooks are about connection management, and there's not an obvious spot to hook "request done".

Tracking issue: https://github.com/iand675/hs-opentelemetry/issues/8

[snoyberg]

I'd be open to adding such hooks to make instrumentation possible, with the caveat that we need to avoid any backwards incompatible changes. Also be aware that the current "hooks" system (if you can even call it that) is a bit of a mess, but hopefully that mess wouldn't impede this kind of change.

[jship]

Some food for thought: a minimally-invasive way to understand timing might be to update withResponse such that it also calls mWrapException like responseOpen currently does, e.g:

withResponse req man f = bracket (responseOpen req man) responseClose f

could become

withResponse req man f =
  mWrapException man req $ bracket (responseOpen req man) responseClose f

That would allow for instrumentation to understand timing of the entire time the response is open, assuming the user/library is using httpLbs/withResponse and not using responseOpen + brRead and friends directly. The downside is that the burden would be on instrumentation if it needed to delineate between invocations (i.e. the new "outer" call of mWrapException in withResponse, or the existing "inner" call of mWrapException within responseOpen).

With ManagerSettings being in the Settings types style, could also backwards-compatibly add a new wrapper function specific to this case. At that rate, perhaps withResponse's definition itself could be configured via ManagerSettings. That would have the added benefit of allowing for a little more convenient request modification to propagate headers too, as opposed to separately using managerModifyRequest.

[snoyberg]

I'd rather avoid (ab)using mWrapException more. This is part of the ugliness I mentioned before. If I was willing to make a breaking release, I'd probably overhaul the mismashed setup we have right now. Instead, it would be best if we didn't interleave the fragile exception handling with instrumentation.