Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add EventSource docs #28826

Merged
merged 5 commits into from
Mar 31, 2022
Merged

Add EventSource docs #28826

merged 5 commits into from
Mar 31, 2022

Conversation

noahfalk
Copy link
Member

@noahfalk noahfalk commented Mar 28, 2022

This is an initial set of docs for using EventSource. The goal was to
capture most of the beginner + intermediate material from
https://github.com/Microsoft/dotnet-samples/blob/master/Microsoft.Diagnostics.Tracing/EventSource/docs/EventSource.md
For brevity I dropped some info about lesser used functionality
(event localization and ETW channel support) while adding some
more info on usage outside the PerfView+ETW ecosystem. I also
added information on ActivityIDs which I haven't found documented
anywhere.

After this PR goes in I'd like to trim the EventSource class api-docs
and add a link to this content as the main place to show overall usage. I
also want to add a link to this content from the dotnet-samples github
page and mark that one as being no longer maintained.

This PR is intended to address
dotnet/diagnostics#2908 - EventSource start/stop
dotnet/diagnostics#2904 - "fast" eventsource
dotnet/diagnostics#2902 - custom EventSource with dotnet-trace+vs
dotnet/diagnostics#1347 - activity ids

It partially covers dotnet/diagnostics#2445 - special case LogAlways behavior
but I think we should do more in the API docs before marking that one complete

Internal previews

This is an initial set of docs for using EventSource. The goal was to
capture most of the beginner + intermediate material from
https://github.com/Microsoft/dotnet-samples/blob/master/Microsoft.Diagnostics.Tracing/EventSource/docs/EventSource.md
For brevity I dropped some info about lesser used functionality
(event localization and ETW channel support) while adding some
more info on usage outside the PerfView+ETW ecosystem. I also
added information on ActivityIDs which I haven't found documented
anywhere.

After this PR goes in I'd like to trim the EventSource class api-docs
and add a link to this content as the main place to show overall usage. I
also want to add a link to this content from the dotnet-samples github
page and mark that one as being no longer maintained.

This PR is intended to address
dotnet/diagnostics#2908 - EventSource start/stop
dotnet/diagnostics#2904 - "fast" eventsource
dotnet/diagnostics#2902 - custom EventSource with dotnet-trace+vs
dotnet/diagnostics#1347 - activity ids

It partially covers dotnet/diagnostics#2445 - special case LogAlways behavior
but I think we should do more in the API docs before marking that one complete
@noahfalk noahfalk marked this pull request as ready for review March 28, 2022 12:50
@noahfalk noahfalk requested review from tommcdon and a team as code owners March 28, 2022 12:50
@noahfalk
Copy link
Member Author

There were some numbered lists in the collect-and-view page that keep repeating "1." I tried replacing that with numbers that count upwards 1,2,3... but then the linter complains that they aren't expected to increment. I'm not sure how to both make the page look visually correct and make the linter happy?

@noahfalk
Copy link
Member Author

@dotnet/dotnet-diag

Copy link
Member

@IEvangelist IEvangelist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @noahfalk,

I started reviewing this, and then I realized that I was suggesting a lot of changes. Could you please revise this a bit, before I jump back into a full review? Here are the priorities:

  1. The documentation writing style should be active instead of passive. For example:

We can run the code below to see Activity tracking in action.

Becomes:

You can run the code below to see Activity tracking in action.

  1. Do not manually insert new lines, instead let your text editor wrap for you.

Comment on lines +11 to +13
This guide explains Activity IDs, an optional identifier that can be logged with each event generated using
<xref:System.Diagnostics.Tracing.EventSource?displayProperty=nameWithType>. For an introduction see
[Getting Started with EventSource](./eventsource-getting-started.md).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This guide explains Activity IDs, an optional identifier that can be logged with each event generated using
<xref:System.Diagnostics.Tracing.EventSource?displayProperty=nameWithType>. For an introduction see
[Getting Started with EventSource](./eventsource-getting-started.md).
This guide explains Activity IDs, an optional identifier that can be logged with each event generated using <xref:System.Diagnostics.Tracing.EventSource?displayProperty=nameWithType>. For an introduction see [Get started with EventSource](./eventsource-getting-started.md).

Comment on lines 17 to 24
Long ago a typical application may have been simple and single-threaded which makes logging straightforward. We
could write each step to a log file in order and then read the log back exactly in the order it was written to
understand what happened. If the app handled requests then being single threaded only one request was handled
at a time. All log messages for request A would be printed in order, then all the messages for B, and so on.
When apps become multi-threaded that strategy no longer works because multiple
requests are being handled at the same time. However if each request is assigned to a single thread which processes
it entirely we can solve the problem by recording a thread id for each log message. For example a multi-threaded app
might log:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Long ago a typical application may have been simple and single-threaded which makes logging straightforward. We
could write each step to a log file in order and then read the log back exactly in the order it was written to
understand what happened. If the app handled requests then being single threaded only one request was handled
at a time. All log messages for request A would be printed in order, then all the messages for B, and so on.
When apps become multi-threaded that strategy no longer works because multiple
requests are being handled at the same time. However if each request is assigned to a single thread which processes
it entirely we can solve the problem by recording a thread id for each log message. For example a multi-threaded app
might log:
Long ago a typical application may have been simple and single-threaded which makes logging straightforward. We could write each step to a log file in order and then read the log back exactly in the order it was written to understand what happened. If the app handled requests, then being single threaded; only one request was handled at a time. All log messages for request A would be printed in order, then all the messages for B, and so on.
When apps become multi-threaded that strategy no longer works because multiple requests are being handled at the same time. However, if each request is assigned to a single thread which processes it entirely, we can solve the problem by recording a thread identifier for each log message. For example, a multi-threaded app might log the following:

docs/core/diagnostics/eventsource-activity-ids.md Outdated Show resolved Hide resolved
docs/core/diagnostics/eventsource-activity-ids.md Outdated Show resolved Hide resolved
@noahfalk
Copy link
Member Author

noahfalk commented Mar 30, 2022

Thanks @IEvangelist, I'll make a pass.

Do not manually insert new lines, instead let your text editor wrap for you.

This feels awkward if you mean what I think you mean. If there was a 10 line long paragraph at ~100 characters per line are you suggesting the markdown file viewed in notepad would have a 1000 character line with a single newline at the end and a large horizontal scroll bar?
My editor for this was notepad which isn't going to wrap automatically for me. However even if I change to use a different editor that does auto-wrap I've seen issues in the past with this approach:

  • GitHub suggestions are per file line making the comments very coarse and the suggestion text large
  • other developers reading the text in their editors may not have auto-wrapping
  • build errors are more challenging to locate within such long lines

In the past I got requests from other reviewers that they would appreciate if I did not rely on auto-wrapping because it made it awkward for them to work with the text. If there is a general consensus among the doc team that auto-wrapping is the preferred style I'll follow it, but if it is a matter of personal preference then I'd rather leave it as-is.

@noahfalk
Copy link
Member Author

And thanks @gewarren, I see you already made a bunch of improvements : D

@noahfalk
Copy link
Member Author

I notice that in GitHub's side by side view it doesn't show many characters per line which caused it to wrap more aggresively. Switching to the unified view shows far more characters per line, but if it is an issue I could probably find something that reformats the text to wrap with fewer characters per line.

- Use 2nd person (you rather than we)
- Attempted to simplify/clarify language in a few places
- Fixed a few typos
@noahfalk
Copy link
Member Author

I made the editing pass, but didn't change word wrapping at this point. If there is a doc team consensus that newlines should only be inserted at paragraph breaks (or some other heuristic?) then let me know and I will.

@tdykstra
Copy link
Contributor

If there is a general consensus among the doc team that auto-wrapping is the preferred style I'll follow it, but if it is a matter of personal preference then I'd rather leave it as-is.

It's a matter of personal preference, but most of our .NET docs use line breaks only to separate paragraphs.

Change Log from static field to get-only property
@IEvangelist IEvangelist merged commit ac38411 into dotnet:main Mar 31, 2022
@noahfalk
Copy link
Member Author

Thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants