Add an ability to filter and enrich tracing activities#5853
Conversation
vonzshik
force-pushed
the
tracind-add-filter-and-enrich
branch
from
8f3a209 to
43a47b9
Compare
| /// <summary> | ||
| /// Gets or sets a function that provides a span's name on a per <see cref="NpgsqlCommand"/> basis. | ||
| /// </summary> | ||
| public Func<NpgsqlCommand, string?>? ProvideSpanNameForNpgsqlCommand { get; set; } |
There was a problem hiding this comment.
Unless NpgsqlCommand also exposes a property where I could store a custom span name (in which case this function wouldn't be needed in the first place) at the moment of command creation, this isn't going to be particularly helpful in my opinion. Yes, deriving the span name by using the first N characters of the command text would be trivial, but what if I want to use some label - Select customers with payments due, Verify user account, etc.? I'd have to maintain some global dictionary for mapping command texts to these labels, which would be extremely annoying.
There was a problem hiding this comment.
Not really, you can also use Baggage.SetBaggage/Baggage.GetBaggage or even a custom AsyncLocal to pass required info. I'm personally a bit hesitant to add a new property because it's not going to be visible if you're using ADO.NET interfaces +NpgsqlCommand is probably the most allocated object we have and another property there will just make it even more heavy.
There was a problem hiding this comment.
If the callback isn't executed completely out of band with respect to the created activity, then that could work.
There was a problem hiding this comment.
If the callback isn't executed completely out of band with respect to the created activity, then that could work.
Yep, it's called just before we start doing any possible async actions (the same is true for multiplexing) so it definitely should work.
npgsql/src/Npgsql/NpgsqlCommand.cs
Lines 1508 to 1511 in 43a47b9
There was a problem hiding this comment.
Re adding a property to NpgsqlCommand, see related conversation in SqlClient (dotnet/SqlClient#2937, dotnet/SqlClient#2925).
I agree that we should at least start with the callback without adding anything to NpgsqlCommand (and people can e.g. use AsyncLocal) - we can always do that later if we want.
vonzshik
force-pushed
the
tracind-add-filter-and-enrich
branch
from
43a47b9 to
62749c8
Compare
|
I will give someone a pretty giftcard for this PR |
|
@bunkrur I'll aggressively ping other team members offline so we merge this just before the release) |
It sure could. It could also fix other issues we have with tracing/metrics/logging. The reason why I went with a relatively small pr is:
|
…nd "time-to-last-read" events are generated (#5912)
| /// <summary> | ||
| /// Gets or sets a function that provides a span's name on a per <see cref="NpgsqlCommand"/> basis. | ||
| /// </summary> | ||
| public Func<NpgsqlCommand, string?>? ProvideSpanNameForNpgsqlCommand { get; set; } |
There was a problem hiding this comment.
Re adding a property to NpgsqlCommand, see related conversation in SqlClient (dotnet/SqlClient#2937, dotnet/SqlClient#2925).
I agree that we should at least start with the callback without adding anything to NpgsqlCommand (and people can e.g. use AsyncLocal) - we can always do that later if we want.
Co-authored-by: Shay Rojansky <roji@roji.org>
|
Stoked on this thank you |
Continues #5853 Co-authored-by: Nikita Kazmin <vonzshik@gmail.com> Co-authored-by: Nino Floris <mail@ninofloris.com>
4 participants
Closes #4192
Closes #4250
Closes #4228
Closes #4245
Closes #5595