Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Added Additional DiagnosticSource Guidance #29552

Merged
merged 3 commits into from
May 8, 2018
Merged

Added Additional DiagnosticSource Guidance #29552

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e653635
Addeded Additional Guidance
vancem May 7, 2018
19d6df8
Add link to activites doc
vancem May 7, 2018
b3cfc7d
feedback
vancem May 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 25 additions & 3 deletions src/System.Diagnostics.DiagnosticSource/src/DiagnosticSourceUsersGuide.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

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.

marking the begining and end of an interval of time. The key value of Activities is that they
indicate that they represent a DURATION, and they also track what 'caused' them (and thus
logging systems can stitch together a 'causality graph').

* DO - If for some reason you can't use Activities, and your events mark the start and stop of
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
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 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.)

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).

* 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
Expand Down