[http-client] using hooks #144
Conversation
kakkun61
changed the title
managerModifyRequest and managerModifyResponse to implement this instrumentation
…mplement this instrumentation
kakkun61
force-pushed
the
http-client-modifier
branch
from
a192ebe to
b0bfa6b
Compare
|
This is ready to be review! |
kakkun61
changed the title
| import qualified OpenTelemetry.Trace.Core as Otel | ||
|
|
||
|
|
||
| appendModifierToSettings :: (MonadIO m, HasCallStack) => Otel.TracerProvider -> Orig.ManagerSettings -> m Orig.ManagerSettings |
|
|
||
|
|
||
| appendModifierToSettings :: (MonadIO m, HasCallStack) => Otel.TracerProvider -> Orig.ManagerSettings -> m Orig.ManagerSettings | ||
| appendModifierToSettings tracerProvider settings = withFrozenCallStack $ liftIO $ do |
There was a problem hiding this comment.
I don't think freezing the call stack helps here?
|
|
||
| requestModifier :: HasCallStack => Otel.Tracer -> ThreadLocalStorage -> Orig.Request -> IO Orig.Request | ||
| requestModifier tracer tls request = do | ||
| spanRef <- TLS.getTLS tls |
There was a problem hiding this comment.
So, there's some kind of important set of assumptions here about how requests and responses are handled. It looks to me like you're assuming that a response to a request will always be exactly the next response processed by the thread that sent the response. Is that true? It could do with some explanation as to why we think it is true if so!
(That assumption is important because it means we can use a thread-local storage to effectively identify a request: if a response comes back and there is something in the storage, it must be for the request that we sent.)
There was a problem hiding this comment.
That point is certainly very important. I had made that assumption unconsciously. I will check it out. Maybe I need to think of another way.
| case maybeSpan of | ||
| Just span_ -> do | ||
| Otel.addAttributes span_ $ makeResponseAttributes response | ||
| Otel.endSpan span_ Nothing |
There was a problem hiding this comment.
What happens if there is no response? or if an exception is thrown? I think this version is less safe in the presence of failures than the old one. The old one used the inSpan functions, so should never leave us with unterminated spans, but I think this one can do?
| makeThreadLocalStorage = TLS.mkTLS $ newIORef Nothing | ||
|
|
||
|
|
||
| makeRequestAttributes :: Orig.Request -> Otel.AttributeMap |
There was a problem hiding this comment.
These changes look useful, though!
What
managerModifyRequestandmanagerModifyResponseto implement this instrumentationManagerWhy
How
ManagerSettingprovides hooks for requests and responsescreateSpanin a hook for a request and to callendSpanin a hook for a responseThreadLocalStorageis used to transport span data created in a hook for a request to a hook for a responseNote
Cachix @sandydoo seems to implement a similar instrumentation.
https://github.com/cachix/hs-opentelemetry-instrumentation-http-client/