Added Additional DiagnosticSource Guidance #29552
Conversation
vancem
changed the title
| * 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 |
There was a problem hiding this comment.
Nit: Indentation of raw text does not match the rest of the doc.
| 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.
Nit: "a interval" => "an interval"
| 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 | ||
| indicate that they represent a DURATION, and they also track what 'causeed' them (and thus |
| * 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 | ||
| indicate that they represent a DURATION, and they also track what 'causeed' them (and thus | ||
| logging systems can stitch together a 'causality graph'. |
| indicate that they represent a DURATION, and they also track what 'causeed' them (and thus | ||
| logging systems can stitch together a 'causality graph'. | ||
|
|
||
| * DO - If for some reason you can't use Activies, and your events mark the start and stop of |
|
|
||
| ### 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.
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.
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.
It seems like a significant disadvantage of tuples would be the temptation for the listener to cast the object to ValueTuple<T1, T2, T3> (then use t.Item1, t.Item2, etc.), which would make adding a new data member a breaking change.
(A workaround is to cast to ITuple and use (string) t[0], (int) t[1], but this is non-obvious (e.g., the OP suggested "Item1/2/3") and causes boxing.)
| * 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.
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.
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.
Added Additional DiagnosticSource Guidance Commit migrated from dotnet/corefx@e274d0a
@brianrob
More information on payload guidance.