-
Notifications
You must be signed in to change notification settings - Fork 826
Adding EventName to LogRecord #6306
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #6306 +/- ##
==========================================
- Coverage 86.72% 86.64% -0.09%
==========================================
Files 258 258
Lines 11879 11881 +2
==========================================
- Hits 10302 10294 -8
- Misses 1577 1587 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
|
69a64d5
to
ff31fb8
Compare
ff31fb8
to
0bca940
Compare
@@ -12,6 +12,10 @@ Notes](../../RELEASENOTES.md). | |||
write position, resulting in gRPC protocol errors. | |||
([#6280](https://github.com/open-telemetry/opentelemetry-dotnet/pull/6280)) | |||
|
|||
* **Breaking change**: If `EventName` is specified either through `ILogger` |
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.
I suggest to rephrase this.
- Given this is a stable package, there cannot be breaking changes.
- However, event-name was only exported when enabling the experimental_feature_flag "OTEL_DOTNET_EXPERIMENTAL_OTLP_EMIT_EVENT_LOG_ATTRIBUTES".
The change here is, ILogger's EventName is now exported by default, as LogRecord's EventName. "OTEL_DOTNET_EXPERIMENTAL_OTLP_EMIT_EVENT_LOG_ATTRIBUTES" feature is still required to export EventId.Id as before.
(Not sure of the log-bridge part whether it supported it before or not. If new capability, this is just a feature enhancement, not a breaking change)
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.
done
src/OpenTelemetry.Api/CHANGELOG.md
Outdated
@@ -6,6 +6,8 @@ Notes](../../RELEASENOTES.md). | |||
|
|||
## Unreleased | |||
|
|||
* Added the `EventName` property to `LogRecordData` ([#6306](https://github.com/open-telemetry/opentelemetry-dotnet/pull/6306)) |
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.
maybe make it explicit that this feature (entire log bridge) is still under experimental ?
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.
done
There was a previous conversation on this topic. Please ensure that everything mentioned there is addressed: #4754. |
As far as this goes, I only made it so that |
71ea9a5
to
4267ca3
Compare
@@ -12,6 +12,13 @@ Notes](../../RELEASENOTES.md). | |||
write position, resulting in gRPC protocol errors. | |||
([#6280](https://github.com/open-telemetry/opentelemetry-dotnet/pull/6280)) | |||
|
|||
* If `EventName` is specified either through `ILogger` or the experimental |
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.
Good writeup!
src/OpenTelemetry/Logs/LoggerSdk.cs
Outdated
@@ -35,6 +36,7 @@ public override void EmitLog(in LogRecordData data, in LogRecordAttributeList at | |||
|
|||
logRecord.Data = data; | |||
logRecord.ILoggerData = default; | |||
logRecord.ILoggerData.EventId = new EventId(0, data.EventName); |
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.
nit: Use something to indicate 0 is a conscious default choice.
new EventId(default, data.EventName)
does this work?
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.
Sure, that makes sense.
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.
LGTM. (I recommend to get more eyes before merging, as I am not fully familiar with the experimental bridge side of things.)
src/OpenTelemetry.Api/CHANGELOG.md
Outdated
@@ -6,6 +6,10 @@ Notes](../../RELEASENOTES.md). | |||
|
|||
## Unreleased | |||
|
|||
* Experimental (only in pre-release versions): Added the `EventName` property |
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.
nit: This should be added below the existing entries in the "Unreleased" section
src/OpenTelemetry/Logs/LogRecord.cs
Outdated
/// <summary> | ||
/// Gets or sets the name of the event associated with the log. | ||
/// </summary> | ||
public string? EventName |
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.
Do we need this? Export authors can access EventName via existing logRecord.EventId.Name
(which they should already be doing). Concerns are it is more public API to maintain and it could introduce confusion in the API.
An alternative approach could be: a) Leave this out for now. b) Wait for EventId
to land in the spec. c) Once that happens obsolete LogRecord.EventId
and add spots for official EventName
and EventId
(whatever it ends up being).
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.
That makes sense. Do you know if there is currently a proposal to add EventId to the spec? I couldn't find anything in the repository for semantic conventions.
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.
None that is open/active. (open-telemetry/semantic-conventions#372 is the old one)
A lot has changed in the past few months (EventName being a top-level field is a new thing!), and I think it's best to create a new issue to propose adding EventId.Id to spec/convention
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.
Because there is some ambiguity in translating ILogger events to OpenTelemetry events, we're discussing an idea to provide an "event name resolver". We need to make a decision about this design before moving forward with this PR.
@juliuskoval What is the main thing you want? Do you require support via ILogger, or do you mainly want support from the experimental bridge API? If the latter, then one suggestion is that you could reduce the scope of this PR to leave the ILogger behavior alone for now while we settle on the design. Either way we can discuss this further at the next SIG meeting.
I mostly cared about the bridge API but I'm not in a hurry, so we can just wait. But either way I'll try to join the next SIG meeting. |
Addresses #6108
Changes
I added a new property to
LogRecord
calledEventName
.Added a field called
EventName
to theLogRecordData
struct which enables specifyingEventName
when using the log bridge API.The OTLP exporter exports
EventName
according to the proto definition.Merge requirement checklist
CHANGELOG.md
files updated for non-trivial changes