Skip to content

[Proof of Concept] Azure Monitor - Refactoring the EventSource #16061

Merged
rajkumar-rangaraj merged 4 commits intoAzure:masterfrom
rajkumar-rangaraj:rajrang/eventsource
Oct 20, 2020
Merged

[Proof of Concept] Azure Monitor - Refactoring the EventSource #16061
rajkumar-rangaraj merged 4 commits intoAzure:masterfrom
rajkumar-rangaraj:rajrang/eventsource

Conversation

@rajkumar-rangaraj
Copy link
Contributor

@rajkumar-rangaraj rajkumar-rangaraj commented Oct 18, 2020

Continuation of #14781

TODO

  • Test case


[NonEvent]
public void SdkVersionCreateFailed(Exception ex)
public void Write(string name, object value)

Choose a reason for hiding this comment

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

For context, this is being used like so:
AzureMonitorTraceExporterEventSource.Log.Write($"FailedToExport{EventLevelSuffix.Error}", ex.LogAsyncException());

This is a cool idea, but I'm not a fan of this approach.
Mainly because this requires string parsing, manipulation inside the EventSource to determine the EventLevel detail.
The event level detail was known at compile time and I think this step can be avoided.
Plus, this assumes that the period ('.') won't be used as punctuation in the message itself.

I would propose either,

  • call the correct method directly
  • or pass an enum into this method and switch on the enum. (this avoids the string parsing).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an inspiration from DiagnosticSource class, StartActivity and StopActivity
Ref: https://github.com/dotnet/runtime/blob/6072e4d3a7a2a1493f514cdf4be75a3d56580e84/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DiagnosticSourceActivity.cs#L30

I'm using a very simple string parser in EventSource, where execution should be fast with no data allocation.

Choose a reason for hiding this comment

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

I think the solution used by DiagnosticSource isn't valid here.
DiagnosticSource is communicating across applications and can't share any managed types, therefore it's storing all the data into that string.

In our example, our project can call this method directly and pass in an Enum, which would avoid the string parsing completely.

Copy link

@TimothyMothra TimothyMothra Oct 20, 2020

Choose a reason for hiding this comment

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

Additionally, because of LastIndexOf('.') this method will break the first time someone tries to write one or more sentences into this error message (using a period as punctuation). This limitation isn't referenced in the summary for this method. :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I merged the changes, will create a new PR to address it.

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM.

@rajkumar-rangaraj rajkumar-rangaraj merged commit 2dc9ff4 into Azure:master Oct 20, 2020
annelo-msft pushed a commit to annelo-msft/azure-sdk-for-net that referenced this pull request Feb 17, 2021
…#16061)

* Changed eventsource

* Check for IsEnabled

* Fine tune

* Fine tune
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants