[Logs-branch] More spec-compliant handling of InstrumentationScope #3762
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main-logs #3762 +/- ##
=============================================
+ Coverage 86.99% 87.39% +0.40%
=============================================
Files 299 299
Lines 10631 10670 +39
=============================================
+ Hits 9248 9325 +77
+ Misses 1383 1345 -38
|
| @@ -46,7 +46,7 @@ public override void EmitEvent(string name, in LogRecordData data, in LogRecordA | |||
| { | |||
| Guard.ThrowIfNullOrWhitespace(name); | |||
|
|
|||
| string eventDomain = this.EnsureEventDomain(); | |||
| this.EnsureEventDomain(); | |||
There was a problem hiding this comment.
Fine as a TODO to follow up on, but throwing here should probably be vetted with the spec. I think normally we'd not allow for exceptions in this circumstance. Spec says:
Events require the event.domain attribute. The API MUST not allow creating an Event if the Logger instance doesn't have event.domain scope attribute.
This statement should be clarified. This might be a scenario where the SDK should log, but not throw an exception.
There was a problem hiding this comment.
This seemed weird to me as well! I asked about it, here's the reply I got:
open-telemetry/opentelemetry-specification#2768 (comment)
I'll drop a comment with that link in there for now.
| @@ -33,29 +33,73 @@ internal static class LogRecordExtensions | |||
| { | |||
| private static readonly string[] SeverityTextMapping = new string[] | |||
| { | |||
| null, "Trace", "Debug", "Information", "Warning", "Error", "Fatal", | |||
| "Trace", "Debug", "Information", "Warning", "Error", "Fatal", | |||
There was a problem hiding this comment.
Something to come back to in another PR maybe...
I think these are somewhat inspired by a combination of ILogger and the LogRecord data model... but technically, I think SeverityText is supposed to be the name as it's known by the logging framework being used see: https://github.com/open-telemetry/opentelemetry-proto/blob/724e427879e3d2bae2edc0218fff06e37b9eb46e/opentelemetry/proto/logs/v1/logs.proto#L144-L146
The SeverityNumber (which I don't think we're using today) on the other hand is governed by the data model. But even then, each logging framework should probably optionally be responsible for defining any mapping from its severity levels to one of the normalized OTLP severity numbers. See https://github.com/open-telemetry/opentelemetry-proto/blob/724e427879e3d2bae2edc0218fff06e37b9eb46e/opentelemetry/proto/logs/v1/logs.proto#L140-L142
There was a problem hiding this comment.
Wow interesting! At the moment on this branch, you have to specify a spec value or null:
That makes it the appender's job to map to spec a la:
We'll have to make changes to support passing the raw original log level as a string in addition to the spec value. Is the raw non-normalized value useful? 🤣
There was a problem hiding this comment.
Is the raw non-normalized value useful?
Yea, hard to say, maybe?
If you've been using some logging framework for 30 years, you're probably pretty accustomed to its nomenclature and not so much OpenTelemetry's. So, I guess it might make analyzing logs more familiar to an end user like this?
Changes
Previously we were setting
event.domainon eachLogRecord. The spec actually says it should be added to theInstrumentationScopeassociated with theLoggeremitting theLogRecord.