Library instrumentation guidance with DiagnosticSource

[lmolkova]

We are looking for help from the tracing community to review out library instrumentation guidance

https://github.com/microsoft/ApplicationInsights-dotnet/blob/develop/WEB/LibraryInstrumentationGuidance.md

Start of the discussion:
StackExchange/StackExchange.Redis#833

We will be happy to hear your feedback about anything, some exiting concerns:

• no configuration
• no contract
• Tags vs payload usage
• lack of detail
• efficiency/performance
• concrete vs anonymous types

We hope to come up with the guidance that would work well with library owners and tracing systems.
Our philosophy for tracing still has to follow some principles like

• automagical with (almost) zero configuration
• (almost) zero perf impact when nobody listens to, and runtime sampling and verbosity control when there is a listener
• no additional dependencies, so .NET is traceable itself
• ability to build rich UI on top of it and run our important customer scenarios

We'll publish the result in more official place.

/cc @cwe1ss, @NickCraver, @stebet, @vancem

Update: I will submit the first PR later this week emphasizing concrete payload types, proposing PascalCase and would provide mode details about sampling/verbosity

[stebet]

Here are some of my ideas after the discussions that have already taken place in several other issues. By all means chime in and let's try to get everyone on the same page on how to work with this, as I think all of us can see the potential and would love to see that potential realized.

Here are some points to get things started :)

What to do with current code that does not adhere to todays guidelines? (ASP.NET, HttpClient, SqlCommand to name a few)

1. Convert anonymous payloads into strongly typed objects

   This should be possible with little chance of breaking backwards compatibility as long as the field names match the anonymous object if I'm not mistaken, since reflection is required anyways to fetch the values. As long as they are named the same, things should not break.

2. New versioned events to get up to spec?

   In the case of for example HttpClient, could a new event called System.Net.Http.HttpRequestOut.V2 or somesuch (naming scheme debatable) be created? This is already being done in the DiagnosticsHandler which is already sending two events, one of which is deprecated.

   I know it isn't optimal to sprinkle more diagnostic code into code that already has it but that would ideally be a temporary solution until deprecated events can be removed, after being clearly marked as such using the [Deprecated] attribute (if using strongly typed objects as above) as well as the possibility to use something like AppContext switches to turn them off for performance reasons.

How can we help everyone write good events and payloads?

1. Standardize naming

   Self-explanatory.

2. Standardize Activity.Tags

   Activity.Tags needs to be standardized if hey are to be of any use. A suggestion has been made to standardize on the OpenTracing tag naming scheme. To me that sounds like a good starting point for ideas, but might be be refined.

   My opinion on Tags is that they should summarize the most commonly used values that are beneficial for logging and basic profiling purposes. I know that's making a lot of assumptions, but some sane middle ground should be possible.

   Examples of possible Tag population

   System.Data.SqlCommand (or even System.Data.DbCommand?):

   activity.Tags.Add("component", "System.Data.SqlClient");
   activity.Tags.Add("db.instance", "master");
   activity.Tags.Add("db.statement", "SELECT * FROM sys.tables");
   activity.Tags.Add("db.type", "sql");
   activity.Tags.Add("db.user", "sa");
   activity.Tags.Add("peer.address", "mysqlserver.mydomain.something:1433"); // might be redundant if hostname and port are populated
   activity.Tags.Add("peer.hostname", "mysqlserver.mydomain.something"); // might be redundant if address is populated
   activity.Tags.Add("peer.port", "1433"); // might be redundant if address is populated
   activity.Tags.Add("peer.ipv4", "192.168.128.10"); // redundant if ipv6
   activity.Tags.Add("peer.ipv4", "2001:0db8:85a3:0000:0000:8a2e:0370:7334"); // redundant if ipv4
   ...

   StackExchange.Redis:

   activity.Tags.Add("component", "StackExchange.Redis");
   activity.Tags.Add("db.instance", "0");
   activity.Tags.Add("db.statement", "SET mykey myvalue");
   activity.Tags.Add("db.type", "redis");
   activity.Tags.Add("peer.hostname", "myredisserver.mydomain.something:6379"); // good if it could tell which node (master/slave/cluster node) it's hitting,
   ...

   System.Net.HttpClient:

   activity.Tags.Add("component", "System.Net.HttpClient");
   activity.Tags.Add("http.method", "GET");
   activity.Tags.Add("http.status_code", "200");
   activity.Tags.Add("http.url", "https://www.google.com/search?q=DiagnosticSource+Activity+Guidance");
   ...

3. Performance/allocation considerations

   To reduce Tag size and allocations some options/settings to define "verbosity" should be encouraged, like log levels. That might be complicating things, but I feel you should be at least to also turn them off completely in case you are working with the payloads and don't care about the tags, sinceall the same information should be available in the payload objects as well. That way consumers can avoid the extra processing and allocations that come with populating the tags.

4. Payload vs Activity.Tags

   Explain very clearly the differences between these, when to use which and the pros and cons as well as impact on performance.

5. Document best practices and common scenarios for both publishers and subscribers

   Have detailed resources on how all of this works and best practices with detailed examples that cover scenarios ranging from basic functionality to performance and allocation considerations. This is in my opinion one of the most important things in all of this. And for that to have any effect, Microsoft must follow these guidelines themselves.

6. Maintain helpers/extensions to both produce and consume events and encourage 3rd parties to do the same for their libraries

   If at all possible, create helpers and extensions libraries to make it easier for both consumers and publishers to use these events. Also encourage 3rd party authors to build it into their libraries to make consumption easier. Also, if feasible, create code analysers that help maintain best practices.

These are my initial thoughts on all this and hopefully can get the discussion started on where to take all this. I'd love to see more ideas as my ideas are of course influenced by my own experience and experimentation and use-cases. I'm sure there are many other things that need to be thought through.

[bratan05]

I agree with you.

[lmolkova]

I have started to work on it in microsoft/ApplicationInsights-dotnet-server#917`.

@stebet thanks for your input!
I'm working on some of your point (all related to documentation/guidance).
Regarding the API changes: helpers, verbosity level, we will discuss it internally.

Some comments:

What to do with current code that does not adhere to todays guidelines

We have discussions about it with some .NET teams and faced a strong pushback against concrete payload types. On the other side .NET producers have guarantees for backward compatibility similar to public APIs. We cannot make all producers change the instrumentation and doing so does not bring any new features, i.e. it's unlikely it will have enough priority.

Microsoft must follow these guidelines themselves.

Right, but I think some ships have sailed and so we can follow them with the new instrumentations.

How can we help everyone write good events and payloads?

In general I agree with your points. Libraries should define tags and should use opentracing approach. The problem is that e.g. opencensus defines other set of http tags. So there is no standard here. I'm also concerned what guidance to give to a 'new' unknown library that have to choose tags on their own.

[cwe1ss]

Thanks for creating this issue and the documentation. Here's some general things. I'll add my feedback regarding the actual content to microsoft/ApplicationInsights-dotnet-server#917.

• Will this be merged with the existing DiagnosticSourceUserGuide / ActivityUserGuide? There's a lot of overlap and they currently all have outgoing HTTP calls as examples but they're all using different instrumentation code.

  • If not, this really shouldn't be in the Application Insights repository as it is a vendor-neutral topic (and even less people will find it)

• Will all of this be moved to docs.microsoft.com anytime soon? This is really important. The only thing I found about general tracing on .NET is this and that's only talking about Debug, Trace and so on. ASP.NET Core is doing a much better job here - they even created documentation for IHostingStartup which probably is relevant only to a very small amount of people.

  • IMO, there should be a big "Tracing" section under .NET Guide that gives people clear guidance on when & how to use EventSource, EventCounter, DiagnosticSource, Activity, Microsoft.Extensions.Logging and all the other stuff.

[zihotki]

I wonder how this aligns with the new OpenCensus buzz? The question may be a bit premature but given that MS joined OpenCensus and there is some progress with .Net implementation but anyway what is the current strategy?

[vancem]

It is our intent that OpenCensus support should be orthogonal to DiagnosticSource. The events generated by DiagnosticSource should work fine in OpenCensus. Until we have end-to-end scenarios fully deployed and used, there can always be issues, but that is our intent/design so far.

[lucaspimentel]

With the repo migration, the new link to the document is:
https://github.com/microsoft/ApplicationInsights-dotnet/blob/develop/WEB/LibraryInstrumentationGuidance.md

[github-actions]

This issue is stale because it has been open 300 days with no activity. Remove stale label or comment or this will be closed in 7 days.