SqlCommand: Add new Tag property

[perlun]

This can be used to provide user-specified context for a particular SqlCommand instance. See
#2931 for more context.

[perlun]

(These whitespace-related changes are there because I have my editor set to remove trailing newlines on save. I can revert those hunks if you feel like it)

[perlun]

On the placement of this: it would ideally be placed in the higher-level DbCommand class, but putting it there feels like it's going to make it a bigger effort (and perhaps take much longer to be included in a deployed release). Are we fine with placing this here, in the SqlCommand class itself?

[roji]

I'd advise against quickly adding an arbitrary new object property without it actually doing anything, and without complete clarity on how it fits into the diagnostics/telemetry story. For one thing, someone will come along who wants to have two tags instead of just one, which this wouldn't support.

[Wraith2]

Agreed on the object type being bad. For general use I'd say string and if we only expect it to be used as a lookup key then int or guid would be my choice.

[edwardneal]

I agree with roji's point about this being best with its own issue to discuss - in part because I'd want to thread this through the metrics too.

To focus specifically on the content on this PR:

• It seems sensible to use the built in TagList type for the property.
• Separately, Name seems to be a common enough telemetry requirement that it might justify its own property.
• If we have it on the normal SqlCommand, I think we should also have it on SqlBatchCommand.
• Tentatively, it might be helpful to treat this approach as a general rule of thumb for any actionable public API - SqlConnection, SqlDataReader, etc. The lifecycle/inheritance of these tags would remain to be discussed.
• Whichever classes receive it, the corresponding classes in the ref projects also need the same definition.

[Wraith2]

I've used TagList recently and it's a horrible type to work with. It's huge, it's an immutable struct and trying to create or edit the contents in an enrichment scenario is painful. I understand the original motivation behind the type but it really doesn't work very well in practice.

[edwardneal]

I've just checked, and it looks like it's just the metrics which deal directly with TagList parameters, in Counter<T>.Add(T, in TagList) et al. Everything else can work with sets of KeyValuePair<string, object?>, so the Tags property could just as easily be that.

WRT immutability: the notion that the tags of a SqlCommand could change underneath us in an async call sounds like a bad idea. Is there a use case for using a read-write property instead of a read-only property and an extra constructor?

[Wraith2]

My idea for this was for a tag, not tags. If people want to do otel stuff with a tag then that's fine but I wasn't suggesting we add otel types directly to the SqlCommand. Perhaps my choice of tag as the name is a source of confusion. Would Identifier? be a better option? The intention from me was that we allow people creating and using SqlCommand objects to contain a simple (hence, not object) piece of metadata which they can pick up in the diagnostics either log directly or use a fast lookup to get a value to log (or trace, or meter).

The tags of an SqlCommand are unlikely to change. The tags of a meter can easily change. My comment was directed strictly at the TagList type.
Consider the example of a multitenant application using the http.request.duration metric that asp.net provides. on each metric recorded it is likely that you want to add in the tenant identifier so you can track requests by tenant. I did this. It was awful code to write because every time I tried to do sensible performant things with TagList it wasn't possible. Lookup a key? nope, got to enumerate it yourself. Replace something to update the tag value? nope, immutable. Pass it around? immutable so you have to use ref and return or out parameters and be very careful.

[edwardneal]

My idea for this was for a tag, not tags. If people want to do otel stuff with a tag then that's fine but I wasn't suggesting we add otel types directly to the SqlCommand. Perhaps my choice of tag as the name is a source of confusion. Would Identifier? be a better option? The intention from me was that we allow people creating and using SqlCommand objects to contain a simple (hence, not object) piece of metadata which they can pick up in the diagnostics either log directly or use a fast lookup to get a value to log (or trace, or meter).

It makes sense, and I understand thanks. The originating issue seems to be opentelemetry-dotnet-contrib/1792, where the goal is to pass the new property through to the span's display name, which is where Identifier becomes logical (and corresponds to the Name property I was thinking about, although mine's probably more focused on bridging the telemetry gap to OTel and might be more confusing for SqlClient developers.)

I've suggested using an IEnumerable<KeyValuePair<string, object?>> type (either instead of, or in addition to, a specific property - both approaches work) because only passing through an identifier might be too restrictive. If someone in the future wanted to associate another tag with the trace or a metric (perhaps a tenant name) then they're left in a difficult position - it's probably too specific to their use case to merit a new property, but adding a broader SqlCommand.CustomTags property to store these custom tags could introduce questions of precedence between those tags and the existing Identifier/Name property.

The tags of an SqlCommand are unlikely to change. The tags of a meter can easily change. My comment was directed strictly at the TagList type.

I've seen some of the use cases you've mentioned. Your comment about the TagList type just reminded me to re-check the original PR, and I commented on that. Sorry for the confusion there!

[roji]

Interesting discussion, I've been keeping this mind for Npgsql as well - maybe there's a cross-provider thing to be done. But for SqlClient I'd recommend first doing the basic OTel support (both metrics and tracing) and only then going into advanced stuff like this, where the user can inject custom tags per command.

On the specific API shape, I'm leaning in the direction of @edwardneal, i.e. allow users to manipulate multiple tags, rather than just a single identifier, which won't be sufficient for some purposes... Assuming you decide to go ahead with native support for OTel, exposing OTel-specific things on SqlCommand may be OK.

[perlun]

I'd advise against quickly adding an arbitrary new object property without it actually doing anything, and without complete clarity on how it fits into the diagnostics/telemetry story.

That's a valid point @roji, but... it solves an immediate problem we have right now. Given the sealed nature of SqlCommand, it's unnecessarily hard to "shoe-horn" metrics-related arbitrary (or for that matter, any form of arbitrary data) into the current architecture.

Do you happen to know why the class is sealed in the first place, I guess it's in line with the framework design considerations (to better enforce encapsulation)?

For one thing, someone will come along who wants to have two tags instead of just one, which this wouldn't support.

Well, you can always put a Tuple<string, string> there if you need. Or Dictionary<string, object>. 🙂

I think my idea with making it an arbitrary object is to avoid limiting the usefulness of the field. It's really an "arbitrary", user-defined field, that can be used for "anything".

Once we have a better view of what's needed for telemetry, we could add another (more strongly-typed) field for one or more telemetry-metadata-related requirements. That could then be something like a CommandTelemetryMetadata record instance or similar.

[roji]

it solves an immediate problem we have right now.

Sure, but what works from your very specific perspective doesn't necessarily makes sense as a public API addition for everyone, on a type like SqlCommand.

As in #2931 (comment), I think the AsyncLocal approach should be sufficient, at least until we see evidence that something more is needed.

[benrr101]

As per the issue, we're interested in developing a plan for this that more adequately integrates with our OpenTelemetry plans as well as aligns with the plans of our neighbor, npgsql. As such we won't be taking this PR in it's current state

please see discussion #2937

[perlun]

Sure, but what works from your very specific perspective doesn't necessarily makes sense as a public API addition for everyone, on a type like SqlCommand.

I can buy into that @roji, particularly as we want to keep the API stable. Adding an arbitrary field that might be suggested to be removed later on, might not be a great UX for the users of this API.

It would still be interesting to hear the rationale behind this, though. 👇

Do you happen to know why the class is sealed in the first place, I guess it's in line with the framework design considerations (to better enforce encapsulation)?

It limits the extensibility of the class a bit. We already have a wrapper class around SqlCommand in fact, which adds some extra functionality but because of the sealed nature of the class, we are forced to use composition instead of inheritance. And in the telemetry-related use case (Enrich) we don't get the instance of "our" wrapper class of course, only the "raw" SqlCommand, which is why this turned out to be a problem for us.

As in #2931 (comment), I think the AsyncLocal approach should be sufficient, at least until we see evidence that something more is needed.

Indeed, we went with the AsyncLocal approach for now. It seems to work, at least for our current use cases. If we run into cases where it doesn't, I'll ask again to see if anyone has ideas on how we can solve it. Closing the PR for now as it won't be merged as-is.

[perlun]

@edwardneal @Wraith2 The discussion will continue here: #2937. It could be worth copying some of your points from this PR into that discussion, to make it more visible.