Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public override ExportResult Export(in Batch<Activity> batch)
}
catch (Exception ex)
{
AzureMonitorTraceExporterEventSource.Log.FailedExport(ex);
AzureMonitorTraceExporterEventSource.Log.Write($"FailedToExport{EventLevelSuffix.Error}", ex.LogAsyncException());
return ExportResult.Failure;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT License.

using System;
using System.Collections.Generic;
using System.Diagnostics.Tracing;

namespace OpenTelemetry.Exporter.AzureMonitor
Expand All @@ -11,41 +12,64 @@ internal sealed class AzureMonitorTraceExporterEventSource : EventSource
{
private const string EventSourceName = "OpenTelemetry-TraceExporter-AzureMonitor";
public static AzureMonitorTraceExporterEventSource Log = new AzureMonitorTraceExporterEventSource();

[NonEvent]
public void FailedExport(Exception ex)
public readonly IReadOnlyDictionary<string, EventLevel> EventLevelMap = new Dictionary<string, EventLevel>
{
if (this.IsEnabled(EventLevel.Error, EventKeywords.All))
{
this.FailedExport(ex.ToInvariantString());
}
}
[EventLevelSuffix.Critical] = EventLevel.Critical,
[EventLevelSuffix.Error] = EventLevel.Error,
[EventLevelSuffix.Warning] = EventLevel.Warning,
[EventLevelSuffix.Informational] = EventLevel.Informational,
[EventLevelSuffix.Verbose] = EventLevel.Verbose
};

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

{
if (this.IsEnabled(EventLevel.Warning, EventKeywords.All))
{
this.WarnSdkVersionCreateException(ex.ToInvariantString());
}
}
var lastIndex = name.LastIndexOf('.');
var eventLevel = EventLevelMap[name.Substring(lastIndex)];

[NonEvent]
public void ConnectionStringError(Exception ex)
{
if (this.IsEnabled(EventLevel.Error, EventKeywords.All))
if (this.IsEnabled(eventLevel, EventKeywords.All))
{
this.ConnectionStringError(ex.ToInvariantString());
var message = $"{name.Trim(lastIndex)} - {GetMessage(value)}";

switch (eventLevel)
{
case EventLevel.Critical:
WriteCritical(message);
break;
case EventLevel.Error:
WriteError(message);
break;
case EventLevel.Informational:
WriteInformational(message);
break;
case EventLevel.Verbose:
WriteVerbose(message);
break;
case EventLevel.Warning:
WriteWarning(message);
break;
}
}
}

[Event(1, Message = "Failed to export activities: '{0}'", Level = EventLevel.Error)]
public void FailedExport(string exception) => this.WriteEvent(1, exception);
[Event(1, Message = "{0}", Level = EventLevel.Critical)]
public void WriteCritical(string message) => this.WriteEvent(1, message);

[Event(2, Message = "{0}", Level = EventLevel.Error)]
public void WriteError(string message) => this.WriteEvent(2, message);

[Event(2, Message = "Error creating SdkVersion : '{0}'", Level = EventLevel.Warning)]
public void WarnSdkVersionCreateException(string message) => this.WriteEvent(2, message);
[Event(3, Message = "{0}", Level = EventLevel.Warning)]
public void WriteWarning(string message) => this.WriteEvent(3, message);

[Event(3, Message = "Connection String Error: '{0}'", Level = EventLevel.Error)]
public void ConnectionStringError(string message) => this.WriteEvent(3, message);
[Event(4, Message = "{0}", Level = EventLevel.Informational)]
public void WriteInformational(string message) => this.WriteEvent(4, message);

[Event(5, Message = "{0}", Level = EventLevel.Verbose)]
public void WriteVerbose(string message) => this.WriteEvent(5, message);

private static string GetMessage(object value)
{
return value is Exception exception ? exception.ToInvariantString() : value.ToString();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@ public async ValueTask<int> TrackAsync(IEnumerable<TelemetryItem> telemetryItems
{
// TODO: Network issue. Send Telemetry Items To Storage
}
// TODO: Log the exception to new event source. If we get a common logger we could just log exception to it.
AzureMonitorTraceExporterEventSource.Log.FailedExport(ex);

AzureMonitorTraceExporterEventSource.Log.Write($"FailedToSend{EventLevelSuffix.Error}", ex.LogAsyncException());
}

return itemsAccepted;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public static void GetValues(string connectionString, out string instrumentation
}
catch (Exception ex)
{
AzureMonitorTraceExporterEventSource.Log.ConnectionStringError(ex);
AzureMonitorTraceExporterEventSource.Log.Write($"ConnectionStringError{EventLevelSuffix.Error}", ex);
throw new InvalidOperationException("Connection String Error: " + ex.Message, ex);
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

namespace OpenTelemetry.Exporter.AzureMonitor
{
internal class EventLevelSuffix
{
public const string Critical = ".Critical";
public const string Error = ".Error";
public const string Warning = ".Warning";
public const string Informational = ".Informational";
public const string Verbose = ".Verbose";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,10 @@ internal static string ToInvariantString(this Exception exception)
Thread.CurrentThread.CurrentUICulture = originalUICulture;
}
}

internal static Exception LogAsyncException(this Exception exception)
{
return exception is AggregateException aggregateException ? aggregateException.InnerException : exception;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ private static string GetSdkVersion()
}
catch (Exception ex)
{
AzureMonitorTraceExporterEventSource.Log.SdkVersionCreateFailed(ex);
AzureMonitorTraceExporterEventSource.Log.Write($"SdkVersionCreateFailed{EventLevelSuffix.Warning}", ex);
return null;
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System;

namespace OpenTelemetry.Exporter.AzureMonitor
{
internal static class StringExtensions
{
public static string Trim(this string message, int lastIndex)
{
return message.Substring(0, lastIndex);
}
}
}