Drop event bus message spans #3238
Conversation
Signed-off-by: Carsten Lohmann <[email protected]>
Signed-off-by: Carsten Lohmann <[email protected]>
| // Suppression of these spans via "quarkus.opentelemetry.tracer.suppress-non-application-uris" doesn't work | ||
| // if the non-application-root-path is the same as the overall root path. | ||
| return new DropHttpRequestSpansSampler(sampler); | ||
| sampler = new DropHttpRequestSpansSampler(sampler); | ||
| // drop spans for the given event bus message prefixes |
There was a problem hiding this comment.
I wonder if we might want to actually use the non-application root path then. Given that we already decided that the 2.0.0 chart will not be backward compatible with Hono 1.x, I think that we could as well take advantage of the existing features then ...
There was a problem hiding this comment.
Right, we could do that. (The DropHttpRequestSpansSampler would still be needed though for now, as not only the non-application-root-path spans shall get dropped. Issue for the removal of that sampler is #3213.)
Changing the non-application-root-path and the healthcheck endpoints should be done in a separate PR then.
There was a problem hiding this comment.
as not only the non-application-root-path spans shall get dropped.
Couldn't we simply remove the TracingHandler in favor of the standard functionality provided by the quarkus-opentelemetry extension?
There was a problem hiding this comment.
That is basically the goal of #3213 (see also #3205 (comment)).
It's a bit more than just removing the TracingHandler class. You mean including this in 2.0.0?
I began working on this but then stopped, waiting for the Quarkus update. I can have another look.
There was a problem hiding this comment.
Well, if we want to use the non-application-root path for probes and metrics, then I think we should do it with 2.0.0. Not sure how this is related to getting rid of the TracingHandler, but if we can kill two birds with one stone, then I think we should do it.
There was a problem hiding this comment.
In any case, this PR here is mostly about the event bus spans. I've only included a small fix for the DropHttpRequestSpansSampler regarding the Quarkus update (and the changed span names there).
There was a problem hiding this comment.
Changing the non-application-root-path and the healthcheck endpoints should be done in a separate PR then.
I take it that we indeed want to change these paths with Hono 2.0.0 so that they include a non-empty non-application-root-path (e.g. q), right?
There was a problem hiding this comment.
With regards to the dropping of the healthcheck endpoint spans, the current Hono code is independent from the used non-application-root-path. As a custom sampler is needed in Hono (for using the SamplingPrioritySampler), dropping of these spans needs to be done manually anyway (until a solution for quarkusio/quarkus#25525 is implemented). So, mandating usage of the /q healthcheck endpoint prefix would only enable us to simplify the span-dropping logic a bit here.
In that sense, I have no strong view about switching to use the /q prefix. But I guess, as Quarkus is now used for all Hono components and we don't need Helm-chart backwards-compatibility here, we might as well stick to the Quarkus default paths.
Follow up of #3219:
With the new Quarkus version, traces for Vert.x event bus messages now get created (by means of EventBusInstrumenterVertxTracer). This means the adapter traces will now include traces like
notification.tenant-change-v1 sendwithnotification.tenant-change-v1 receivechild spans (and corresponding traces for device and credentials changes). And the device registry/command router will containauthentication.in sendtraces (withauthentication.in receivechild span).These traces just represent an implementation detail on how notification messages and the authentication processing are handled and don't offer real value. Therefore, this PR drops these traces.
Note that the usage of
TracingPolicy.IGNOREto prevent creation of such traces (as described in the Vert.x documentation) doesn't work with the current Quarkus version - see quarkusio/quarkus#25417.