Skip to content

Conversation

josalem
Copy link
Contributor

@josalem josalem commented Sep 2, 2021

  • reduces focus on ETW
  • Explains difference between self describing and manifest event formats
  • adds more complex examples
  • explains conventions for a well designed EventSource
  • remove references to 4.5 and 4.6 to reduce reading complexity
  • remove note about IDisposable

Part of dotnet/diagnostics#515

* reduces focus on ETW
* Explains difference between self describing and manifest event formats
* adds more complex examples
* explains conventions for a well designed EventSource
* remove references to 4.5 and 4.6 to reduce reading complexity
* remove note about IDisposable
@josalem josalem requested review from a team, brianrob and davmason September 2, 2021 21:11
@josalem josalem self-assigned this Sep 2, 2021
@josalem josalem requested review from a team, Anipik, ViktorHofer and tarekgh as code owners September 2, 2021 21:11
@ghost
Copy link

ghost commented Sep 2, 2021

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti
See info in area-owners.md if you want to be subscribed.

Issue Details
  • reduces focus on ETW
  • Explains difference between self describing and manifest event formats
  • adds more complex examples
  • explains conventions for a well designed EventSource
  • remove references to 4.5 and 4.6 to reduce reading complexity
  • remove note about IDisposable

Part of dotnet/diagnostics#515

Author: josalem
Assignees: josalem
Labels:

area-System.Diagnostics.Tracing

Milestone: -

Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

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

LGTM

@josalem
Copy link
Contributor Author

josalem commented Sep 2, 2021

Does it matter that the "Snippet 5000" check keeps failing? As far as I can tell, all the code snippets will cause this check to fail. I can't seem to spot a project file for any of them or one in a parent project that would associate them all together. Any guidance on that?

@gewarren
Copy link
Contributor

gewarren commented Sep 2, 2021

Docs Build status updates of commit f94b69e:

⚠️ Validation status: warnings

File Status Preview URL Details
xml/System.Diagnostics.Tracing/EventSource.xml ⚠️Warning View Details
samples/snippets/csharp/VS_Snippets_CLR/etwtrace/cs/program.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR/etwtracelarge/cs/program.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR/etwtracesmall/cs/program.cs ✅Succeeded View

xml/System.Diagnostics.Tracing/EventSource.xml

  • Line 0, Column 0: [Warning-xref-not-found] Cross reference not found: 'System.Tracing.EventSourceSettings.EtwManifestEventFormat'.
  • Line 0, Column 0: [Warning-xref-not-found] Cross reference not found: 'System.Diagnostics.Tracing.EventSource.ctor(string)'.
  • Line 0, Column 0: [Warning-xref-not-found] Cross reference not found: 'System.Diagnostics.Tracing.EventSource.ctor(string, EventSourceSettings)'.
  • Line 0, Column 0: [Warning-xref-not-found] Cross reference not found: 'Syste.Diagnostics.Tracing.EventSource.Write'.

For more details, please refer to the build report.

If you see build warnings/errors with permission issues, it might be due to single sign-on (SSO) enabled on Microsoft's GitHub organizations. Please follow instructions here to re-authorize your GitHub account to Docs Build.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

Note: Your PR may contain errors or warnings unrelated to the files you changed. This happens when external dependencies like GitHub alias, Microsoft alias, cross repo links are updated. Please use these instructions to resolve them.

For any questions, please:

@gewarren
Copy link
Contributor

gewarren commented Sep 2, 2021

Docs Build status updates of commit 7e97cfe:

⚠️ Validation status: warnings

File Status Preview URL Details
xml/System.Diagnostics.Tracing/EventSource.xml ⚠️Warning View Details
samples/snippets/csharp/VS_Snippets_CLR/etwtrace/cs/program.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR/etwtracelarge/cs/program.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR/etwtracesmall/cs/program.cs ✅Succeeded View

xml/System.Diagnostics.Tracing/EventSource.xml

  • Line 0, Column 0: [Warning-xref-not-found] Cross reference not found: 'System.Tracing.EventSourceSettings.EtwManifestEventFormat'.
  • Line 0, Column 0: [Warning-xref-not-found] Cross reference not found: 'System.Diagnostics.Tracing.EventSource.ctor(string)'.
  • Line 0, Column 0: [Warning-xref-not-found] Cross reference not found: 'System.Diagnostics.Tracing.EventSource.ctor(string, EventSourceSettings)'.
  • Line 0, Column 0: [Warning-xref-not-found] Cross reference not found: 'Syste.Diagnostics.Tracing.EventSource.Write'.

For more details, please refer to the build report.

If you see build warnings/errors with permission issues, it might be due to single sign-on (SSO) enabled on Microsoft's GitHub organizations. Please follow instructions here to re-authorize your GitHub account to Docs Build.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

Note: Your PR may contain errors or warnings unrelated to the files you changed. This happens when external dependencies like GitHub alias, Microsoft alias, cross repo links are updated. Please use these instructions to resolve them.

For any questions, please:

@josalem
Copy link
Contributor Author

josalem commented Sep 3, 2021

Is there any way to xref directly to a specific .ctor or member on a class?

@tarekgh
Copy link
Member

tarekgh commented Sep 3, 2021

Is there any way to xref directly to a specific .ctor or member on a class?

Here are some examples for class, property, method, and constructor.

<see cref="T:System.Globalization.CultureInfo" />
<see cref="P:System.Globalization.CultureInfo.Name" />
<see cref="M:System.Globalization.DateTimeFormatInfo.GetEraName(System.Int32)" />
<see cref="Overload:System.Globalization.CultureInfo.#ctor" />

@gewarren
Copy link
Contributor

gewarren commented Sep 3, 2021

Docs Build status updates of commit cbf84de:

⚠️ Validation status: warnings

File Status Preview URL Details
xml/System.Diagnostics.Tracing/EventSource.xml ⚠️Warning View Details
samples/snippets/csharp/VS_Snippets_CLR/etwtrace/cs/program.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR/etwtracelarge/cs/program.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR/etwtracesmall/cs/program.cs ✅Succeeded View

xml/System.Diagnostics.Tracing/EventSource.xml

  • Line 0, Column 0: [Warning-xref-not-found] Cross reference not found: 'System.Diagnostics.Tracing.EventSource.ctor(string)'.
  • Line 0, Column 0: [Warning-xref-not-found] Cross reference not found: 'System.Diagnostics.Tracing.EventSource.ctor(string, EventSourceSettings)'.
  • Line 0, Column 0: [Warning-xref-not-found] Cross reference not found: 'Syste.Diagnostics.Tracing.EventSource.Write'.

For more details, please refer to the build report.

If you see build warnings/errors with permission issues, it might be due to single sign-on (SSO) enabled on Microsoft's GitHub organizations. Please follow instructions here to re-authorize your GitHub account to Docs Build.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

Note: Your PR may contain errors or warnings unrelated to the files you changed. This happens when external dependencies like GitHub alias, Microsoft alias, cross repo links are updated. Please use these instructions to resolve them.

For any questions, please:

@gewarren
Copy link
Contributor

gewarren commented Sep 3, 2021

Does it matter that the "Snippet 5000" check keeps failing? As far as I can tell, all the code snippets will cause this check to fail. I can't seem to spot a project file for any of them or one in a parent project that would associate them all together. Any guidance on that?

Yes, please add a simple project file similar to https://github.com/dotnet/dotnet-api-docs/blob/main/snippets/System.Text/Rune/csharp/RuneSamples.csproj. This is a requirement that we added in the last month, which is why most of the snippets don't have one yet.

John Salem and others added 2 commits September 3, 2021 14:04
Co-authored-by: Genevieve Warren <24882762+gewarren@users.noreply.github.com>
@gewarren
Copy link
Contributor

gewarren commented Sep 3, 2021

Docs Build status updates of commit 528c95c:

✅ Validation status: passed

File Status Preview URL Details
samples/snippets/csharp/VS_Snippets_CLR/etwtrace/cs/program.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR/etwtracelarge/cs/program.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR/etwtracesmall/cs/program.cs ✅Succeeded View
xml/System.Diagnostics.Tracing/EventSource.xml ✅Succeeded View

For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

@gewarren
Copy link
Contributor

gewarren commented Sep 3, 2021

Docs Build status updates of commit 4e2a074:

✅ Validation status: passed

File Status Preview URL Details
samples/snippets/csharp/VS_Snippets_CLR/etwtrace/cs/etwtrace.csproj ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/etwtrace/cs/program.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR/etwtracelarge/cs/program.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR/etwtracelarge/etwtracelarge.csproj ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/etwtracesmall/cs/etwtracesmall.csproj ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/etwtracesmall/cs/program.cs ✅Succeeded View
xml/System.Diagnostics.Tracing/EventSource.xml ✅Succeeded View

For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

@gewarren
Copy link
Contributor

gewarren commented Sep 3, 2021

Docs Build status updates of commit 0ade23d:

✅ Validation status: passed

File Status Preview URL Details
samples/snippets/csharp/VS_Snippets_CLR/etwtrace/cs/etwtrace.csproj ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/etwtrace/cs/program.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR/etwtracelarge/cs/program.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR/etwtracelarge/etwtracelarge.csproj ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/etwtracesmall/cs/etwtracesmall.csproj ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/etwtracesmall/cs/program.cs ✅Succeeded View
xml/System.Diagnostics.Tracing/EventSource.xml ✅Succeeded View

For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

@opbld32
Copy link

opbld32 commented Sep 7, 2021

Docs Build status updates of commit 77b00ce:

✅ Validation status: passed

File Status Preview URL Details
samples/snippets/csharp/VS_Snippets_CLR/etwtrace/cs/etwtrace.csproj ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/etwtrace/cs/program.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR/etwtracelarge/cs/program.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR/etwtracelarge/etwtracelarge.csproj ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/etwtracesmall/cs/etwtracesmall.csproj ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/etwtracesmall/cs/program.cs ✅Succeeded View
xml/System.Diagnostics.Tracing/EventSource.xml ✅Succeeded View

For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

@josalem
Copy link
Contributor Author

josalem commented Sep 7, 2021

Before I merge this, is there any way to change the order that the sections of the doc are rendered in? The preview starts with the examples and advanced usage sections despite those being lower in the markdown. It would make more sense for the remarks and conventions sections to be higher up.

if (IsEnabled())
{
EventSource.EventData* descrs = stackalloc EventSource.EventData[3];
descrs[0] = new EventData { DataPointer = (IntPtr)(&arg1), Size = 4 };
Copy link
Member

@hoyosjs hoyosjs Sep 7, 2021

Choose a reason for hiding this comment

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

Is this a safe thing to do in a sample? Here you are getting them from args only on primitives, which makes it OK. Do we have good guidance around fields of classes for things like strings and other ref types?

Copy link
Contributor Author

@josalem josalem Sep 7, 2021

Choose a reason for hiding this comment

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

Event methods on EventSource must call WriteEvent and their parameters must match. An exception will be thrown if the event method's parameter list doesn't match the parameter list of the WriteEvent overload used. This code is adding an overload of WriteEvent that knows how to quickly write a payload of int, short, long which isn't a pre-written overload.

The EventData struct can only contain types that are capable of being represented with EventSource metadata:

// A Valid type to log to an EventSource include
//   * Primitive data types
//   * IEnumerable<T> of valid types T (this include arrays)  (* New for V4.6)
//   * Explicitly Opted in class or struct with public property Getters over Valid types.  (* New for V4.6)

EventSource derives its metadata from the parameter list of the event method which must match the call to WriteEvent.

In other words, you can only do this optimization with strings and primitive types. You could theoretically write something like this though:

public class MyType
{
	public string Foo { get; set; }
	public int Bar { get; set; }
	public long Baz { get; set; }
}

public abstract class UtilSource : EventSource
{
	protected UtilSource() : base () { }
	protected UtilSource(bool throwOnEventWriteErrors) : base(throwOnEventWriteErrors) { }

	protected unsafe void WriteEvent(int eventId, string arg1, int arg2, long arg3)
	{
		if (IsEnabled())
		{
			fixed (char* _arg1 = arg1 ?? string.Empty)
			{
				EventSource.EventData* descrs = stackalloc EventSource.EventData[3];
				descrs[0] = new EventData { DataPointer = (IntPtr)(_arg1), Size = ((arg1?.Length ?? 0) + 1) * 2 };
				descrs[1] = new EventData { DataPointer = (IntPtr)(&arg2), Size = 4 };
				descrs[2] = new EventData { DataPointer = (IntPtr)(&arg3), Size = 8 };
				WriteEventCore(eventId, descrs);
			}
		}
	}
}

public sealed MySource : UtilSource
{
	public static MySource Log = new();

	[NonEvent]
	public void LogThing(MyType thing) => DumpMyType(thing.Foo, thing.Bar, thing.Baz);

	[Event(1)]
	public void DumpMyType(string arg1, int arg2, long arg3) => WriteEvent(1, arg1, arg2, arg3);
}

Copy link
Member

Choose a reason for hiding this comment

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

Mostly what I meant: should the sample show to save pointers for things that are not safe to assume pinned (strings and other things adhering to the third point of "opted in", including value types that live in the heap like boxed and embedded types). Exactly like the example you have or something like CounterPayload since this is an advanced scenario type of thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can't really do this optimization with anything reference typed besides strings and you need to have the fixed scope surround your call to WriteEventCore or you will run into issues.

The "opted in" option is a reference to types decorated with EventDataAttribute which only applies to the Write<T> API which can't be optimized like this.

@gewarren
Copy link
Contributor

gewarren commented Sep 7, 2021

Before I merge this, is there any way to change the order that the sections of the doc are rendered in? The preview starts with the examples and advanced usage sections despite those being lower in the markdown. It would make more sense for the remarks and conventions sections to be higher up.

Oh that is strange what it did. What about making Conventions, Self-describing (tracelogging) vs. manifest event formats, Examples, and Advanced usage all H3 headings? e.g. ### Examples

@josalem
Copy link
Contributor Author

josalem commented Sep 8, 2021

@gewarren, is there a way to manually trigger the OpenPublishing.Build job? It seems to be stuck.

@gewarren
Copy link
Contributor

gewarren commented Sep 8, 2021

@gewarren, is there a way to manually trigger the OpenPublishing.Build job? It seems to be stuck.

Yes, close/reopen the PR. I'll do it now.

@gewarren gewarren closed this Sep 8, 2021
@gewarren gewarren reopened this Sep 8, 2021
@opbld33
Copy link

opbld33 commented Sep 8, 2021

Docs Build status updates of commit 304e7e9:

✅ Validation status: passed

File Status Preview URL Details
samples/snippets/csharp/VS_Snippets_CLR/etwtrace/cs/etwtrace.csproj ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/etwtrace/cs/program.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR/etwtracelarge/cs/program.cs ✅Succeeded View
samples/snippets/csharp/VS_Snippets_CLR/etwtracelarge/etwtracelarge.csproj ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/etwtracesmall/cs/etwtracesmall.csproj ✅Succeeded
samples/snippets/csharp/VS_Snippets_CLR/etwtracesmall/cs/program.cs ✅Succeeded View
xml/System.Diagnostics.Tracing/EventSource.xml ✅Succeeded View

For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

@josalem
Copy link
Contributor Author

josalem commented Sep 8, 2021

Hmmm still seems to be shuffling the sections.

@gewarren
Copy link
Contributor

gewarren commented Sep 8, 2021

Before I merge this, is there any way to change the order that the sections of the doc are rendered in? The preview starts with the examples and advanced usage sections despite those being lower in the markdown. It would make more sense for the remarks and conventions sections to be higher up.

Oh that is strange what it did. What about making Conventions, Self-describing (tracelogging) vs. manifest event formats, Examples, and Advanced usage all H3 headings? e.g. ### Examples

That didn't do the trick. @mimisasouvanh Any suggestions here about how to change the ordering of the Markdown headings?

@josalem
Copy link
Contributor Author

josalem commented Sep 9, 2021

I'm okay merging it with the current arrangement, but it hampers readability to start with examples rather than remarks.

@gewarren gewarren merged commit 3e26d4d into dotnet:main Sep 10, 2021
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.

7 participants