Refactor tracing configuration API#5928
Conversation
Co-authored-by: Nikita Kazmin <vonzshik@gmail.com>
|
Thanks for the review @vonzshik. Will wait for any input from @NinoFloris before merging as we've been discussing this a lot recently. |
| #region Tracing | ||
|
|
||
| internal void TraceCommandStart(NpgsqlConnectionStringBuilder settings, NpgsqlTracingOptions? tracingSettings) | ||
| internal void TraceCommandStart(NpgsqlConnectionStringBuilder settings, NpgsqlTracingOptions tracingOptions) |
There was a problem hiding this comment.
Yeah - just below? For the filter and span name callbacks?
There was a problem hiding this comment.
Or you mean the word "settings" in the name? If so yeah, I'm switching to "options" as that seems to be the direction we're going?
There was a problem hiding this comment.
Am I looking at different code (did a quick review earlier) I mean the NpgsqlConnectionStringBuilder settings not the tracing options.
There was a problem hiding this comment.
Ah sorry, I was focussed on the tracing options part.
Yeah the connstring builder is there only because we want the database name and we get it out of there. We most probably can get it out of the data source or connector, but let's leave that for another time?
| ILoggerFactory? _loggerFactory; | ||
| bool _sensitiveDataLoggingEnabled; | ||
| NpgsqlTracingOptions? _tracingOptions; | ||
| Action<NpgsqlTracingOptionsBuilder>? _configureTracingOptionsBuilder; |
There was a problem hiding this comment.
So the final shape of this is dependent on our conclusions, right?
There was a problem hiding this comment.
You mean in terms of lazy (keep the lambda (or possibly lambdas) and evaluate at the end), or eager (evaluate the lambda immediately and only keep the resulting options), right?
If so yeah... For now I went with the way you did it in the other type loading PR (lazy). I think whatever we do here is likely fine for now, and we can revisit in 10 and possibly change things - but I'm happy to change now if you prefer.
There was a problem hiding this comment.
Yeah I mostly meant the multi lambda part indeed, whether that's lazy or eager.
There was a problem hiding this comment.
Right, so for now I really just followed what you had in your PR, which was a single lazily-evaluated lambda... Are we good with that for now?
| /// Gets or sets a value indicating whether to enable the "time-to-first-read" event. | ||
| /// Default is true to preserve existing behavior. | ||
| /// </summary> | ||
| public NpgsqlTracingOptionsBuilder EnableFirstResponseEvent(bool enable = true) |
There was a problem hiding this comment.
For things that are enabled by default, has this pattern been confusing to EF users? Where they have to use the enable method to disable a default?
There was a problem hiding this comment.
I understand the hesitation - I haven't seen anybody actually have trouble with it... People who don't want to disable never see it or care, and people who do figure it out...
I'd be OK with flipping this around (DisableFirstResponseEvent), but then you get double-negation passing false, so... Let me know what you prefer.
There was a problem hiding this comment.
Yeah I would definitely stick to the non negated naming convention. Was wondering whether you had gotten any EF feedback calling out (somewhat) worse discoverability.
3 participants
Continues #5853
Some general API thoughts I had while doing this:
AddNpgsql()(from the Npgsql.OpenTelemetry package), but is configured on a per-datasource basis on NpgsqlDataSourceBuilder.TracerProviderBuilder.AddSource()needs to be called for any sort of collection to take place (this is a global thing).TracerProviderBuilderis defined in the OpenTelemetry.API package, which Npgsql should not reference (see these docs).TracerProviderBuilder.AddSource(); but this would require the user to pass in their TracerProviderBuilder, which departs from the way tracing is usually configured etc...AddNpgsql()globally (and so all data sources trace), but they can tweak tracing config on NpgsqlDataSourceBuilder.AddSource()activates tracing for all data sources, it may be desirable to have an API on NpgsqlDataSourceBuilder which disables tracing for that data source. We can always add that on NpgsqlTracingOptionsBuilder later if someone asks for it.