Adding EventName to LogRecord#6306
Conversation
github-actions
bot
added
pkg:OpenTelemetry.Api
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.74% 86.76% +0.02%
==========================================
Files 258 258
Lines 11879 11881 +2
==========================================
+ Hits 10304 10309 +5
+ Misses 1575 1572 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
juliuskoval
force-pushed
the
adding-EventName
branch
from
69a64d5 to
ff31fb8
Compare
juliuskoval
force-pushed
the
adding-EventName
branch
from
ff31fb8 to
0bca940
Compare
| 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.
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)
|
|
||
| ## Unreleased | ||
|
|
||
| * Added the `EventName` property to `LogRecordData` ([#6306](https://github.com/open-telemetry/opentelemetry-dotnet/pull/6306)) |
There was a problem hiding this comment.
maybe make it explicit that this feature (entire log bridge) is still under experimental ?
|
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 |
juliuskoval
force-pushed
the
adding-EventName
branch
from
71ea9a5 to
4267ca3
Compare
| 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 |
|
|
||
| logRecord.Data = data; | ||
| logRecord.ILoggerData = default; | ||
| logRecord.ILoggerData.EventId = new EventId(0, data.EventName); |
There was a problem hiding this comment.
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.
Sure, that makes sense.
cijothomas
left a comment
cijothomas
left a comment
There was a problem hiding this comment.
LGTM. (I recommend to get more eyes before merging, as I am not fully familiar with the experimental bridge side of things.)
|
|
||
| ## Unreleased | ||
|
|
||
| * Experimental (only in pre-release versions): Added the `EventName` property |
There was a problem hiding this comment.
nit: This should be added below the existing entries in the "Unreleased" section
6 participants
Fixes #6108
Changes
I added a new property to
LogRecordcalledEventName.Added a field called
EventNameto theLogRecordDatastruct which enables specifyingEventNamewhen using the log bridge API.The OTLP exporter exports
EventNameaccording to the proto definition.Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes