Support for enriching activities#4284
Conversation
roji
left a comment
roji
left a comment
There was a problem hiding this comment.
Thanks, this looks great! See mostly minor comments below.
One thing I'm slightly concerned about, is that Npgsql will probably support other activities, and not just command execution (e.g. physical connection open, COPY, prepare...). Because of this, having Enrich correspond to command execution could be problematic, so maybe something like EnrichCommandExecution?
| configure?.Invoke(options); | ||
| return builder | ||
| .AddSource("Npgsql") | ||
| .AddInstrumentation(() => new NpgsqlTracingInstrumentation(options)); |
There was a problem hiding this comment.
I don't know the OpenTelemetry very well - what's the advantage of AddInstrumentation with NpgsqlTracingInstrumentation here over simply injecting NpgsqlActivitySource.Options directly in this method?
There was a problem hiding this comment.
I think we only want to set NpgsqlActivitySource.Options if and when Build() is called on the TraceProviderBuilder. I also think technically we want to remove those options once the built TracerProvider has been disposed. Using an instrumentation factory that returns an IDisposable gives us that behaviour.
(See 3.i here)
There was a problem hiding this comment.
Isn't that documentation section only relevant when the instrumentation library is separate/external, because the main library isn't instrumented? In other words, I think what we're doing qualifies as part of "make every library observable out of the box by having them call OpenTelemetry API directly" (the first sentence).
On a related note, I don't think this config stuff qualifies as "state management", which is what I think the AddInstrumentation API is about.
But I'm far from an expert on all this - let me know how you see things.
There was a problem hiding this comment.
I think regardless of if the documentation section is intended to be for just separate instrumentations, the behaviour it gives us is desirable here: only set NpgsqlActivitySource.Options if and when Build() is called on the TraceProviderBuilder, and unset it when the TraceProvider is disposed.
- nullability of options - documentation
tl-david-hayden
force-pushed
the
otel-enrich
branch
from
dea520d to
172f1db
Compare
|
Hey thanks so much for taking the time to review.
I've updated as suggested - it might be worth also adding filtering when there are multiple types of spans generated, as I suspect some users might only be interested in a subset, especially if they are paying per span ingested. |
|
@roji You mentioned this being for v7.0 - what is the release schedule for that? |
Absolutely - we'll need to think about that when we add those extra activities.
We're aligned with the .NET release schedule, so 7.0 should go out in November - a bit far away, I know. In theory we could do a 6.1 release, but that complicates things and I'd rather avoid it unless there are some very good reasons. Note also that I have various other plans for OpenTelemetry and diagnostics in general - so it's good to have some time on this. |
roji
left a comment
roji
left a comment
There was a problem hiding this comment.
Thanks @Haydabase, looks almost ready; just details around the tuple parameter etc.
|
@Haydabase pinging to make sure this doesn't get forgotten :) It's really almost ready :) |
|
@Haydabase sorry, unfortunately I'm currently busy with other things... I'll definitely be back to concentrating on OpenTelemetry before 7.0, but it will likely happen later in the release cycle... |
# Conflicts: # Directory.Packages.props
|
@roji Sorry for another ping, just thinking it is likely less than a month to the v7 release - is this likely to get in? |
|
This PR was superseded by #5853 which was released in Npgsql 9.0. Thanks for the attempt @Haydabase, don't hesitate to open issues if you run into limitations of the current functionality. |
4 participants
This PR addresses #4192, using a similar
Enrichsignature to the Http tracing instrumentation (here).Documentation PR: npgsql/doc#177