[WIP] Using ILogger instead of EventSource for logging OpenTelemetry Events in OTLP exporter #5879
Conversation
github-actions
bot
added
documentation
| @@ -73,12 +73,6 @@ public void CouldNotTranslateMetric(string className, string methodName) | |||
| this.WriteEvent(5, className, methodName); | |||
| } | |||
|
|
|||
| [Event(8, Message = "Unsupported value for protocol '{0}' is configured, default protocol 'grpc' will be used.", Level = EventLevel.Warning)] | |||
There was a problem hiding this comment.
This change is not related to logging migration. Removed because this method was not referenced.
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpLogExporter.cs
Outdated
Show resolved
Hide resolved
| { | ||
| // TODO: there's no concept of Event vs NonEvent in ILogger (as opposed to EventSource.) | ||
| // Need to find a way to log "NonEvent" in ILogger to avoid the below situation: | ||
| // When the Exception occurred in ExportMethod, the Export event got written to the pipe, and thus the Exporter keeps exporting. |
There was a problem hiding this comment.
SupressInstrumentationScope should be preventing infinite loop already.. is that what you are referring to there?
export() -> ilooger.log(exception) -> gets-back-to-export export() -> ilogger.log(exception) -- this infinite cycle?
There was a problem hiding this comment.
Yes, the above call sequence is what I'm referring to. Let me see how SuppressInstrumentationScope can prevent this from happening. Thanks!
| @@ -29,7 +29,8 @@ | |||
| appBuilder.Services.AddSingleton<Instrumentation>(); | |||
|
|
|||
| // Clear default logging providers used by WebApplication host. | |||
There was a problem hiding this comment.
Its not clear to me how the change in the example app relates to the remainder of the PR?
Is this just for your local testing and should be ignored?
There was a problem hiding this comment.
The change in the example app is just for local testing purposes while I'm working on the integration tests for this.
| Debug.Assert(exporterOptions != null, "exporterOptions was null"); | ||
| Debug.Assert(sdkLimitOptions != null, "sdkLimitOptions was null"); | ||
| Debug.Assert(experimentalOptions != null, "experimentalOptions was null"); | ||
|
|
||
| this.openTelemetryEventLogger = openTelemetryEventLogger!; | ||
|
|
||
| this.openTelemetryEventLogger.LogDebug("Hello from Otlp ctor"); |
There was a problem hiding this comment.
Are these for your own debugging and going to come out? If not you might want to adjust the messages, for example "Entering OtlpLogExporter constructor".
There was a problem hiding this comment.
Yes, .LogDebug() is for debugging purposes and will be removed in the upcoming iterations.
Yun-Ting
force-pushed
the
yunl/ToILoggerOTLP
branch
from
52a2598
to
07ac9d5
Compare
|
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
github-actions
bot
added
the
Stale
Towards #3881
Changes
Currently internal logging with ILogger is default on.
Need to find a way to have the internal diagnostics off by default and a switch to turn the diagnostics on.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes