Change xml docs for Activity.OperationName

[cwe1ss]

The new Activity type from DiagnosticSource is based on OpenTracing's span type. However, I've noticed that Activity's meaning of "OperationName" is different than OpenTracing's.

Definition from the OpenTracing specification:

An operation name, a human-readable string which concisely represents the work done by the Span (for example, an RPC method name, a function name, or the name of a subtask or stage within a larger computation). The operation name should be the most general string that identifies a (statistically) interesting class of Span instances. That is, "get_user" is better than "get_user/314159".

For example, here are potential operation names for a Span that gets hypothetical account information:

Operation Name	Guidance
get	Too general
get_account/792	Too specific
get_account	Good, and account_id=792 would make a nice Span tag

With Activity however, the OperationName is used as a compile-time constant that is used for the DiagnosticSource event names - e.g. "System.Net.Http.HttpRequestOut".

Depending on how much you'd like to follow the OpenTracing standard, it might be good to rename the property to reduce confusion. I think that your usage might come closest to OpenTracing's standard tag "component" however using that might be confusing as well. Maybe it's best to call it just Name (and activityName if used as a parameter)?! In the HttpClient instrumentation code you already use ActivityName anyway.

Also, the comment for Activity.OperationName still contains an explanation for an OpenTracing-like usage and therefore should be changed as well.

/// <summary>
/// An operation name is a COARSEST name that is useful grouping/filtering. 
/// The name is typically a compile-time constant.   Names of Rest APIs are
/// reasonable, but arguments (e.g. specific accounts etc), should not be in
/// the name but rather in the tags.  
/// </summary>
public string OperationName { get; }

/cc @lmolkova @vancem

[vancem]

I don't actually see why Activity's operation Name is not exactly Span's concept. They suggested a name like 'get_account' and that is basically like 'HttpRequestOut' used for activity. I don't see the mismatch.

(Note that the actual guidance is to use short names (HttpReqestOut is better than System.Net.Http.HttpRequestOut but fully qualified names don't really hurt, so we don't enforce that).

[cwe1ss]

"get_account" is a part of a specific requested URL. "HttpRequestOut" is a constant for all requests.

[cwe1ss]

The following example might help. In OpenTracing, the operation name tells you more about the actual context of the action. This is used for displaying it in tracers and it also helps to do more fine grained reports.

With Activity you currently only get a trace that looks like this.

• "Microsoft.AspNetCore.HttpRequestIn"
  • "System.Net.HttpRequestOut"
  • "System.Net.HttpRequestOut"
  • "System.Data.SqlOut".

You have to click on each activity to see the tags with more details. With OpenTracing's sense of "OperationName" you will get something like this:

• "GET /checkout" (component: AspNetCore.RequestIn)
  • "GET /customers/details" (component: HttpRequestOut)
  • "POST /shippingcosts" (component: HttpRequestOut)
  • "SQL SELECT" (component: "SqlClientOut").

Here, the "component" tag is the constant that's the same for every outgoing HTTP request.

[vancem]

I see your distinction, but this is not something wired into Activity. You seem to be saying that we did not instrument HTTP properly (we could have used Activity to produce the operation names you indicate above). This may be a fair critique of the instrumentation site, but I actually don't know how HTTP instrumentation along can do what you want above, because it SEEMS to be using the HTTP operation (GET, POST), as well as the URL 'Path' but the path might be very specific (it will often include something very specific to a particular request).

[cwe1ss]

What Activity does is different, not necessarily improper - avoiding confusion for people who might look at both systems is my main point here.

There are advantages and disadvantages to both methods.

Activity:

• guarantees a fixed unique compile time constant for every type of instrumentation. This is great for how activity is used right now because this name is the only way for tracers to know what to do with the instrumented object that's passed as an argument (which it must look at to extract useful information - due to missing default tags)
• This name can not be changed per instance because otherwise the subscription code no longer works (the ActivityName is part of the DiagnosticSource-event)
• It does not guarantee something that groups/identifies the actual instance. This means there's no guaranteed field amongst all activities that can be used for a better name or a more fine-grained report that covers all types of activities.

OpenTracing does the opposite:

• It guarantees a single field that may (or may not) contain a instance specific identifier.
• The operation name can be changed. (E.g. MVC could change the name to a [controller]/[action] pattern). However, there's not really a (good) way to access the root span in a lower layer.
• It does not guarantee a compile-time constant identifier for the activity. It has the component-tag but it is not required. It does not really need that right now because it doesn't have a way to send an instrumented object instance as a parameter to the tracer. (which is a great feature of Activity)

Your example for HTTP is very valid! Neither the ASP.NET request middleware nor the HttpClient handler can reliably know how to transform the full URL into a useful operation name as this differs per application/scenario. There are some solutions for this but none of them are perfect:

• Only use the HTTP method (and maybe the domain for outgoing requests). This would at least result in a small set of values.
• Let a deeper layer with more knowledge (e.g. Routing module / MVC module) overwrite the value
• Use some "algorithm" that everyone has to live with (e.g. take the first segment of the path)
• provide a hook for end users (application developers) to let them resolve it however they like.

[cwe1ss]

I've been playing with Application Insights over the last days and they also have an "Operation Name" field which is used by how OpenTracing defines it (e.g. "POST api/values").

Would be great if you could resolve this inconsistency (before the currently assigned 2.1.0 milestone because this would be a breaking change and therefore impossible).

[vancem]

I think at the heart of the issue is that 'OperationName' may mean different things at different levels of abstraction. It would be nice if we all agreed, but logging systems are built up over time by different groups (the owners of the various components and infrastructure), so we have to make cost-benefit choices.

The action item before us in this issue is to determine whether to rename 'OperationName' in System.Activity because another layer of abstraction closer to the user (e.g. AppInsights / OpenTracing) uses the term that is more specific and has different properties.

As you point out it would be a breaking change to do this in 2.1, but frankly it is already enough of a breaking change now. AppInsights code already depends on the current layout, and they ship independently of the .NET Framework. We actually have a lot of experience with how painful API renames can be, so generally speaking the names of things get locked down very early (typically after the API review).

But the bottom line here is that I am not the gatekeeper on things like this. I am generally pretty flexible on naming and will defer to others precisely because it can be painful and contentious and I have better things to do.

My analysis is that the current name 'OperationName' is 'close enough' and in particular the main value of changing it (to make clear that it may not be the same as a property in Application Insights APIs), is low compared to the very real costs of changing it at this point.

But as I as I said the not the person who needs approve this. I am including some others who have a stake in this. If your argument resonates with any of them, then we can discuss just how painful the logistics would be and make a final determination.

@lmolkova @brahmnes @SergeyKanzhelev @avanderhoorn @karelz @terrajobst @brianrob @weshaggard

[cwe1ss]

Thanks for the response Vance! If the change is no longer possible, I'd suggest at least fixing the docs and the comment as they are not correct right now (at least not for me [I'm not a native english speaker]). It should clearly indicate that the property must be a unique compile time identifier for the activity type and that it will be used by DiagnosticSource subscribers to identify the activity. It must not contain any instance-specific data.

[cwe1ss]

Any feedback from the /cc'd people? A change here is impossible after a release.

I still think that changing the docs/code comments is a must-have because they are currently wrong.

Changing the property name is a nice-to-have, it would just remove some confusion. However, there's probably not many usages yet so the impact might be pretty low. (Is anyone other than Application Insights using this?)

[cwe1ss]

I renamed the issue as renaming the property is obviously out of the question now. It's just about fixing the xml docs now IMO.

[brianrob]

@cwe1ss can you point at which documentation you'd like to change?
@vancem, do you have any concerns with doc updates?

[vancem]

Doc updates are fine.

[brianrob]

Moving to Future as this is now a doc bug.

@cwe1ss can you point me to the documentation that you're interested in changing so that I can figure out next steps? Thanks.

[tarekgh]

Closing this issue as we didn't get a response. If the issue id a doc issue, I would suggest to open a new issue in the https://github.com/dotnet/doc repo