-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Added Additional DiagnosticSource Guidance #29552
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -153,22 +153,44 @@ Thus the event names only need to be unique within a component. | |
* DO NOT - name the listener after the Listener (thus something like System.Net.HttpDiagnosticListener | ||
is bad). | ||
|
||
|
||
|
||
#### Event Names | ||
|
||
* DO - keep the names reasonably short (< 16 characters). Keep in mind that event names | ||
are already qualified by the Listener so the name only needs to be unique within a listener. | ||
Short names make `IsEnabled()` faster. | ||
|
||
* DO - use the 'Start' and 'Stop' suffixes for events that define an interval of time. For example | ||
naming one event 'RequestStart' and the another 'RequestStop' is good because tools can use the | ||
convention to determine that the time interval betweeen them is interesting. | ||
* DO - use activities (see [Activity Users Guide](ActivityUserGuide.md)) for events that are | ||
marking the begining and end of a interval of time. The key value of Activities is that they | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: "a interval" => "an interval" |
||
indicate that they represent a DURATION, and they also track what 'causeed' them (and thus | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: causeed => caused |
||
logging systems can stitch together a 'causality graph'. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: missing closing paren |
||
|
||
* DO - If for some reason you can't use Activies, and your events mark the start and stop of | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Activies => Activities |
||
an interval of time, use the 'Start' and 'Stop' suffixes on the events. | ||
|
||
### Payloads | ||
|
||
* DO use the anonymous type syntax 'new { property1 = value1 ...}' as the default way to pass | ||
a payload *even if there is only one data element*. This makes adding more data later easy | ||
and compatible. | ||
|
||
* CONSIDER creating an explicit type for the payload. The main value for doing this is that the | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have any guidance on using tuples rather than using anonymous types or creating custom types? Presumably the languages support for names wouldn't support inference in this case, but Item1/2/3/etc. could be used. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't have any guidance on using tuples, and by omission, they are not preferred (the DO case above is really the guidance). There is a larger issue of whether we should suggest that people use tuples. They have the advantage that you don't need reflection to access them, they have the disadvantage that you lose the names, and there is yet another convention to choose from. The advantages don't seem worth the complexity/confusion. A more interesting piece of guidance is to suggest that people implement the ITuple interface on their explicit class, but our current plan is to expose a fast property fetcher (like the PropertyFetch class in DiagnosticSourceEventSource), which is probably good enough and does not require users to do extra work. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems like a significant disadvantage of tuples would be the temptation for the listener to cast the object to (A workaround is to cast to |
||
receiver can cast the received object to that type and immediately fetch fields (with anonymous types | ||
reflection must be used to fetch fields). This is both easier to program and more efficient. | ||
Thus in scenarios where there is likely high-volume filtering to be done by the logging listener, having | ||
this type available to do the cast is valuable. Note that this type needs to be made public (since | ||
the listener needs to see it), and should be under the namespace System.Diagnostics.DiagnosticSource.PayloadTypes. | ||
Note that if there is doubt about the value DO NOT create an explicit type, as you CAN convert from | ||
an anonymous type to a explicit type compatibly in the future, but once you expose the payload type | ||
you must keep it forever. The payload type should simply have C# 'TYPE NAME {get; set; }' properties | ||
(you can't use fields). You may add new properties as needed in the future. | ||
|
||
* CONSIDER in high volume cases (e.g. > 1K/sec) consider reusing the payload object instead of | ||
creating a new one each time the event is fired. This only works well if you already have locking | ||
or exclusive objects where you can remember the payload for the 'next' event to send easily and | ||
correctly (you are only saving an object allocation, which is not large). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Presumably then there's also guidance that listeners must not hold onto the objects for later use? e.g. I could theoretically imagine a listener decide "for efficiency I'm just going to store the object given to me and then do some batch dump operation at some frequency", but that would be invalidated if the supplied objects were mutable and reused for each event. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We currently have no guidance on what listeners should do, but it certainly should include not modifying anything handed to it, and making no assumptions about what happens to objects after the callback completes (thus no caching of references). Fundamentally listeners are 'meta' object which should ideally do as little as possible and make as few assumptions as possible. It would be good to write this down. |
||
|
||
* CONSIDER - if you have an event that is so frequent that the performance of the logging is | ||
an important consideration, **and** you have only one data item **and** it is unlikely that | ||
you will ever have more data to pass to the event, **and** the data item is a normal class | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Indentation of raw text does not match the rest of the doc.