Skip to content

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

juliuskoval
Copy link
Contributor

@juliuskoval juliuskoval commented Jun 5, 2025

Addresses #6108

Changes

I added a new property to LogRecord called EventName.
Added a field called EventName to the LogRecordData struct which enables specifying EventName when using the log bridge API.
The OTLP exporter exports EventName according to the proto definition.

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

@github-actions github-actions bot added pkg:OpenTelemetry.Api Issues related to OpenTelemetry.Api NuGet package pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package labels Jun 5, 2025
@juliuskoval juliuskoval changed the title Adding event name Adding EventName to LogRecord Jun 5, 2025
Copy link

codecov bot commented Jun 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.64%. Comparing base (1f9fa9f) to head (e23c391).
Report is 2 commits behind head on main.

✅ All tests successful. No failed tests found.

Additional details and impacted files

Impacted file tree graph

@@            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     
Flag Coverage Δ
unittests-Project-Experimental 86.34% <100.00%> (-0.26%) ⬇️
unittests-Project-Stable 86.53% <100.00%> (-0.03%) ⬇️
unittests-Solution 86.60% <100.00%> (+0.17%) ⬆️
unittests-UnstableCoreLibraries-Experimental 85.78% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/OpenTelemetry.Api/Logs/LogRecordData.cs 100.00% <100.00%> (ø)
...etryProtocol/Implementation/ExperimentalOptions.cs 89.47% <ø> (ø)
...ementation/Serializer/ProtobufOtlpLogSerializer.cs 98.49% <100.00%> (ø)
src/OpenTelemetry/Logs/LoggerSdk.cs 100.00% <100.00%> (ø)

... and 4 files with indirect coverage changes

@github-actions github-actions bot added the documentation Documentation related label Jun 5, 2025
@github-actions github-actions bot removed the documentation Documentation related label Jun 5, 2025
@juliuskoval juliuskoval marked this pull request as ready for review June 5, 2025 06:21
@juliuskoval juliuskoval requested a review from a team as a code owner June 5, 2025 06:21
@@ -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`
Copy link
Member

@cijothomas cijothomas Jun 5, 2025

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.

  1. Given this is a stable package, there cannot be breaking changes.
  2. 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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -6,6 +6,8 @@ Notes](../../RELEASENOTES.md).

## Unreleased

* Added the `EventName` property to `LogRecordData` ([#6306](https://github.com/open-telemetry/opentelemetry-dotnet/pull/6306))
Copy link
Member

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@rajkumar-rangaraj
Copy link
Contributor

There was a previous conversation on this topic. Please ensure that everything mentioned there is addressed: #4754.

@juliuskoval
Copy link
Contributor Author

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 EventName will be exported by default according to the spec but I didn't do anything with logrecord.event.id.

@juliuskoval juliuskoval requested a review from cijothomas June 10, 2025 16:37
@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good writeup!

@@ -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);
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that makes sense.

Copy link
Member

@cijothomas cijothomas left a 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.)

@@ -6,6 +6,10 @@ Notes](../../RELEASENOTES.md).

## Unreleased

* Experimental (only in pre-release versions): Added the `EventName` property
Copy link
Member

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

/// <summary>
/// Gets or sets the name of the event associated with the log.
/// </summary>
public string? EventName
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Member

@alanwest alanwest left a 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.

@juliuskoval
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:OpenTelemetry.Api Issues related to OpenTelemetry.Api NuGet package pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants