Add tracing to MessageDb module in an additive manner #348
Conversation
nordfjord
force-pushed
the
otel-msgdb
branch
from
44ee6e2
to
2d183dc
Compare
src/Equinox.MessageDb/MessageDb.fs
Outdated
| @@ -47,7 +54,7 @@ module Log = | |||
| | None -> f log | |||
| | Some retryPolicy -> | |||
| let withLoggingContextWrapping count = | |||
| let log = if count = 1 then log else log |> prop contextLabel count | |||
| let log = if count = 1 then log else Tracing.addTag("eqx.attempts", count); log |> prop contextLabel count | |||
There was a problem hiding this comment.
I've made it so we have 1 span for the all the attempts rather than 1 per.
Npgsql has tracing built-in as well so you can see the individual queries made to the DB as well.
I'd prefer putting it on all spans so I could do queries like show p99 duration by attempt count, but you've voiced a preference for not including the default value of 1
There was a problem hiding this comment.
Have I made it like that? 🤦 I don't think so
There was a problem hiding this comment.
if its cumbersome to query when irregular, attempt 1 is not a problem to have redundantly in there
There was a problem hiding this comment.
Note there is an outer attempt wrt the OCC retry loop, see https://github.com/jet/equinox/blob/master/src/Equinox.Core/Category.fs#L43
if that needs to be passed inwards as an accumulating context, that can be done... (or does there need to be an outer activity?)
There was a problem hiding this comment.
Oh interesting, so we have n*m retries!
There was a problem hiding this comment.
I'd say in the vast majority of cases the OCC retry (with reload) is far more important of course.
But when the read/write retry is needed, it's really needed.
Thanks to Async CTs you can 'simply' enforce a timeout when it all goes horribly wrong ;)
|
Great work paving the way for the other stores, much appreciated |
I'm too used to tracing to not have it 😅 .
Wonder if we can compromise on this one and add tracing in an additive way like here.
There are three activities defined:
Screenshots of the integration tests
