-
Notifications
You must be signed in to change notification settings - Fork 772
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Logs-branch] More spec-compliant handling of InstrumentationScope #3762
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ | |
using System.Collections.Generic; | ||
using System.Diagnostics; | ||
using OpenTelemetry.Internal; | ||
using OpenTelemetry.Trace; | ||
|
||
namespace OpenTelemetry.Logs; | ||
|
||
|
@@ -44,9 +45,13 @@ public LoggerSdk( | |
/// <inheritdoc /> | ||
public override void EmitEvent(string name, in LogRecordData data, in LogRecordAttributeList attributes = default) | ||
{ | ||
// Note: This method will throw if event.name or event.domain is missing | ||
// or null. This was done intentionally see discussion: | ||
// https://github.com/open-telemetry/opentelemetry-specification/pull/2768#discussion_r972447436 | ||
|
||
Guard.ThrowIfNullOrWhitespace(name); | ||
|
||
string eventDomain = this.EnsureEventDomain(); | ||
this.EnsureEventDomain(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
var provider = this.loggerProvider; | ||
var processor = provider.Processor; | ||
|
@@ -65,8 +70,7 @@ public override void EmitEvent(string name, in LogRecordData data, in LogRecordA | |
|
||
Debug.Assert(exportedAttributes != null, "exportedAttributes was null"); | ||
|
||
exportedAttributes!.Add(new KeyValuePair<string, object?>("event.name", name)); | ||
exportedAttributes!.Add(new KeyValuePair<string, object?>("event.domain", eventDomain)); | ||
exportedAttributes!.Add(new KeyValuePair<string, object?>(SemanticConventions.AttributeLogEventName, name)); | ||
|
||
logRecord.Attributes = exportedAttributes; | ||
|
||
|
@@ -104,7 +108,7 @@ public override void EmitLog(in LogRecordData data, in LogRecordAttributeList at | |
} | ||
} | ||
|
||
private string EnsureEventDomain() | ||
private void EnsureEventDomain() | ||
{ | ||
string? eventDomain = this.eventDomain; | ||
|
||
|
@@ -119,7 +123,5 @@ private string EnsureEventDomain() | |
|
||
this.eventDomain = eventDomain; | ||
} | ||
|
||
return eventDomain!; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow interesting! At the moment on this branch, you have to specify a spec value or
null
:opentelemetry-dotnet/src/OpenTelemetry.Api/Logs/LogRecordData.cs
Line 95 in 7419668
That makes it the appender's job to map to spec a la:
opentelemetry-dotnet/src/OpenTelemetry.Extensions.EventSource/OpenTelemetryEventSourceLogEmitter.cs
Lines 209 to 219 in 7419668
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Also see https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model.md#field-severitytext