Add ability to export attributes corresponding to Logrecord.EventId

[vishweshbankwar]

towards #4875
Design discussion issue #

Changes

Introduces OTEL_DOTNET_EXPERIMENTAL_OTLP_EMIT_EVENT_LOG_ATTRIBUTES for exporting corresponding to LogRecord.EventId. The attributes will be exported as logrecord.event.id and logrecord.event.name.

Merge requirement checklist

• CONTRIBUTING guidelines followed (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)

[rajkumar-rangaraj]

LGTM

[codecov]

Codecov Report

Merging #4925 (cfb0cfa) into main (18c85fa) will decrease coverage by 0.29%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

❗ Current head cfb0cfa differs from pull request most recent head ea918cc. Consider uploading reports for the commit ea918cc to get more accurate results

@@            Coverage Diff             @@
##             main    #4925      +/-   ##
==========================================
- Coverage   83.65%   83.36%   -0.29%     
==========================================
  Files         295      295              
  Lines       12325    12333       +8     
==========================================
- Hits        10310    10282      -28     
- Misses       2015     2051      +36     

Flag	Coverage Δ
unittests	83.36% <100.00%> (-0.29%)	⬇️

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

Files	Coverage Δ
...etryProtocol/Implementation/ExperimentalOptions.cs	100.00% <100.00%> (ø)
...rotocol/Implementation/OtlpLogRecordTransformer.cs	92.85% <100.00%> (+5.51%)	⬆️

... and 8 files with indirect coverage changes

[cijothomas]

resolves #4404 too

[cijothomas]

Looks good to ship this under experimental flag, given there is no existing semantic convention for this.

Would be nice to make the changelog more self-contained to help end users navigate these changes more smoothly.

[cijothomas]

resolves #4404 too

@nblumhardt Could you review as well?

[CodeBlanch]

I think we should shy away from using event.id & event.name for the keys. Reason being, there is already some definition applied to those: https://github.com/open-telemetry/semantic-conventions/blob/main/docs/general/events.md#general-event-attributes

event.name is supposed to go with event.domain

Why tie ourselves to that event stuff though? Seems like some other name would work just fine. dotnet.logrecord.event_id + dotnet.logrecord.event_name or whatever.

/cc @alanwest

[alanwest]

event.name is supposed to go with event.domain

Agreed. @vishweshbankwar and I spoke about this yesterday. If we are adopting the event conventions then I think it is inappropriate to only adopt them halfway.

@vishweshbankwar you were going to discuss this with other. Any updates? Any ideas of what to set event.domain to?

The idea I shared was event.domain=ilogger

Why tie ourselves to that event stuff though?

I'm still not a fan of adopting them, however, as I discussed with @vishweshbankwar, I can get behind it if we embrace the spirit of applying the experimental conventions to help drive them forward based on feedback we may learn. What this looks like to me is:

1. We need to embrace the conventions as they currently are - i.e., set event.domain and event.name
2. If we're going to introduce event.id then someone needs to champion moving this into the spec. Though, I'm ok if we add event.id now, so long as there are plans to start the spec conversation.

[CodeBlanch]

LGTM

Going to try and push clarification through the spec on this: open-telemetry/semantic-conventions#372

[nblumhardt]

In .NET, EventId.Id and EventId.Name are generally only unique/useful in the context of an ILogger category:

Event IDs and names should be unique so that different message types can be identified. (source)

So if they were to be translated into event.id and event.name (seems fine to me) then perhaps the category belongs alongside them in an event.* attribute, too?

Putting the category into event.domain instead of dotnet.ilogger.category would make this intent clear and qualify the names/ids nicely:

event.id = 7
event.name = UserCreated
event.domain = Microsoft.AspNetCore.Identity.UI

Unfortunately the definition of event.domain seems to exclude this usage, but, perhaps another event.* attribute can be coined? dotnet.ilogger.category is a bit of an oddity right now and seems like a good candidate for unification into something more general.

[utpilla]

Any ideas of what to set event.domain to?

@alanwest Firstly, I think deciding this would take some more time and discussion and we need not block this PR for that. We don't have any users asking us to export event.domain attribute so this discussion can be continued even after this PR gets merged.

Now if we could go with Proposal A in open-telemetry/semantic-conventions#372, then we wouldn't have to worry about event.domain.

In the case where we have to go forward with Proposal B/ Event semantic conventions, I feel that CategoryName would be a good candidate for event.domain.

The current description for event.domain on the spec says that:

The domain identifies the business context for the events. [1]
[1]: Events across different domains may have same event.name, yet be unrelated events.

CategoryName fits that description well. For example, an ASP .NET Core app could have multiple loggers each with the category name matching the controller name. That offers a logical grouping of the logs emitted by a given controller. Within each controller, we could have defined events and different controllers could potentially have events with the same name.

[alanwest]

CategoryName fits that description well.

We now have two people on this PR who have expressed this intuition. It's also an option @CodeBlanch suggested long ago. Is there a reason we shouldn't set event.domain to CategoryName now?

[vishweshbankwar]

CategoryName fits that description well.

We now have two people on this PR who have expressed this intuition. It's also an option @CodeBlanch suggested long ago. Is there a reason we shouldn't set event.domain to CategoryName now?

Let me take a step back and summarize what I think we should do:

P0 - Resolve issues reported by users

1) Issue of missing attributes

These attributes have been part of the exporter since the very beginning, and not having the ability to get these attributes is disruptive for users.

• See:
  • Discussion
  • Issue

Additionally, users have asked about this offline as well.

2) Issue of name and id being too generic

Issue

This PR addresses both of these feedbacks.

P1 - Stabilize these experimental attributes

All the discussions here on this PR are a good step towards that.

There is already some engagement on the issue opened by @CodeBlanch.

So, in favor of addressing P0, I think we should go ahead with this PR. Additionally, this will help with getting more feedback as well.

We should continue our discussions on P1 on semantic-conventions issue and opentelemetry-dotnet issue.

For the part of setting the category to event.domain, Here is what we need to consider further:

1. The description in the table looks a bit confusing after looking at the example values and based on another statement. This does not match with what the category name suggests

2. OpenTelemetry Specification Issue.

3. Should this be InstrumentationScope name field? The definition matches closely with the Category Name.

That being said, I agree that P1 still remains unaddressed but it should be handled separately from this PR.

[nblumhardt]

Should this be InstrumentationScope name field? The definition matches closely with the Category Name.

Using instrumentation scopes could be suboptimal for libraries/the SDK generating OTLP payloads, since (as far as I understand it) "log records" nest under "scope logs" and so the SDK would need to sort/group events in order to build properly nested/non-degenerate OTLP payloads.

[reyang]

CategoryName fits that description well.

We now have two people on this PR who have expressed this intuition. It's also an option @CodeBlanch suggested long ago. Is there a reason we shouldn't set event.domain to CategoryName now?

+1, event.domain is better than dotnet.ilogger.category (any name that has ilogger is bad IMHO).

[CodeBlanch]

I really think we should shy away from trying to use the "events" spec here. It seems to be going in a much different direction. event.domain may even be dropped. From a comment on the spec issue, this is how we should structure these "events":

"event.domain": "DotNet",
"event.name": "LogEvent",
"event.data":
{
    key: "",
    categoryName: "",
    source: "",
    id: "",
    data: "",
    message: ... // etc
} 

[alanwest]

event.domain is better than dotnet.ilogger.category

I agree. It's especially nasty to have something emitted with ilogger in the name from the OTLP exporter.

I really think we should shy away from trying to use the "events" spec here. ... event.domain may even be dropped.

This was my intuition too, but I am open to trying it and continuing to solicit feedback from the community working on the event conventions. Adopting the event conventions as they are is important because the spec benefits from our participation in adopting them to drive them forward.

[vishweshbankwar]

event.domain is better than dotnet.ilogger.category

I agree. It's especially nasty to have something emitted with ilogger in the name from the OTLP exporter.

I really think we should shy away from trying to use the "events" spec here. ... event.domain may even be dropped.

This was my intuition too, but I am open to trying it and continuing to solicit feedback from the community working on the event conventions. Adopting the event conventions as they are is important because the spec benefits from our participation in adopting them to drive them forward.

This is what the spec says about InstrumentationScope and it matches closely with category definition.

For log sources which define a logger name (e.g. Java Logger Name) the Logger Name SHOULD be recorded as the Instrumentation Scope name.

This is one of the reasons I think we should not use event.domain for log category. Noted other reasons as well here. With InstrumentationScope name set as CategoryName we do not need to emit this attribute separately.

[alanwest]

This is one of the reasons I think we should not use event.domain for log category. Noted other reasons as well here. With InstrumentationScope name set as CategoryName we do not need to emit this attribute separately.

Good point and I agree that CategoryName could be a good candidate for instrumentation scope name. In fact, this approach would allow our handling of CategoryName be shipped stable and we'd get rid of the dotnet.ilogger.category eyesore. For this reason, I recommend removing handling of LogRecord.CategoryName from this PR. Open a new issue to handle CategoryName as the instrumentation scope name.

This may be tricky as mentioned #4925 (comment), but it may be the right thing to do and we'll need to figure out a way to do it performantly.

---

Circling back to address your comment with regards to event id and name.

These attributes have been part of the exporter since the very beginning, and not having the ability to get these attributes is disruptive for users.

No argument. However, this should not be conflated with what this PR is doing. This PR is attempting to adopt the event conventions. I am not aware of any users that have asked for that.

For now, until someone successfully champions for something different in the spec, event.domain is important. The only suggestion I'm left with is hard-coding it to event.domain=ILogger. But again, I am open to others.

Also, side note, if you read open-telemetry/opentelemetry-specification#2994 carefully, you'll note that while the separate event.domain attribute may go away one suggestion is that its value is merged into event.name - e.g., event.domain=k8s/event.name=some_event becomes event.name=k8s.some_event. If we move forward with adopting the event conventions then as this and other issues related to the event conventions evolve, I expect a commitment from this group to keep our work aligned with the spec.

[vishweshbankwar]

This is one of the reasons I think we should not use event.domain for log category. Noted other reasons as well here. With InstrumentationScope name set as CategoryName we do not need to emit this attribute separately.

Good point and I agree that CategoryName could be a good candidate for instrumentation scope name. In fact, this approach would allow our handling of CategoryName be shipped stable and we'd get rid of the dotnet.ilogger.category eyesore. For this reason, I recommend removing handling of LogRecord.CategoryName from this PR. Open a new issue to handle CategoryName as the instrumentation scope name.

This may be tricky as mentioned #4925 (comment), but it may be the right thing to do and we'll need to figure out a way to do it performantly.

Circling back to address your comment with regards to event id and name.

These attributes have been part of the exporter since the very beginning, and not having the ability to get these attributes is disruptive for users.

No argument. However, this should not be conflated with what this PR is doing. This PR is attempting to adopt the event conventions. I am not aware of any users that have asked for that.

For now, until someone successfully champions for something different in the spec, event.domain is important. The only suggestion I'm left with is hard-coding it to event.domain=ILogger. But again, I am open to others.

Also, side note, if you read open-telemetry/opentelemetry-specification#2994 carefully, you'll note that while the separate event.domain attribute may go away one suggestion is that its value is merged into event.name - e.g., event.domain=k8s/event.name=some_event becomes event.name=k8s.some_event. If we move forward with adopting the event conventions then as this and other issues related to the event conventions evolve, I expect a commitment from this group to keep our work aligned with the spec.

addressing #4925 (comment) : This is a good point. However, we already do similar thing for spans and metrics [Spans are grouped by ActivitySource and Metrics are grouped by Meter]. We have a slightly optimized way of achieving this which we could further look in to improving. But the first step should be complying with stable spec.

Not exporting LogRecord.CategoryName is not an option as this is also important information as per the feedback shared by users. I think we should go ahead use InstrumentationScope name for CategoryName. I am ok to try and keep that part experimental for user feedback.

Regarding the event.domain - I am open to using this attribute after understanding the use case for this field. How does setting event.domain to ilogger or any other value helps users?

Edit:

For now, until someone successfully champions for something different in the spec, event.domain is important

Proposal B in open-telemetry/semantic-conventions#372 is proposing event.domain to be optional.

[alanwest]

Regarding the event.domain - I am open to using this attribute after understanding the use case for this field. How does setting event.domain to ilogger or any other value helps users?

These are questions I'm looking to you to answer. As the person recommending we use the event conventions, I'm leaning on you to describe how they're a good match for what you're trying to solve. We can't go into this ignoring the intentions of the folk who designed the conventions. We need to strive to understand their intentions and work with them to determine if we're heading down the right path. I appreciate that an issue was opened to start this discussion, but I think it's important we see it through before charging forth with whatever approach we deem convenient for our purposes.

---

If you're finding applying the event conventions is too nebulous a challenge at this point in time. I have some other ideas for moving forward.

1. Tell users to use a log processor to add the attributes.
   • This has the obvious downside that it's another component to configure and probably a bummer for performance on the logging hot path.
2. Offer experimental environment variables that enable a user to configure the attribute key themselves. Something like:
   • OTEL_DOTNET_EXPERIMENTAL_EXPORT_LOGRECORD_EVENT_ID_AS
   • OTEL_DOTNET_EXPERIMENTAL_EXPORT_LOGRECORD_EVENT_NAME_AS

[vishweshbankwar]

Regarding the event.domain - I am open to using this attribute after understanding the use case for this field. How does setting event.domain to ilogger or any other value helps users?

These are questions I'm looking to you to answer. As the person recommending we use the event conventions, I'm leaning on you to describe how they're a good match for what you're trying to solve. We can't go into this ignoring the intentions of the folk who designed the conventions. We need to strive to understand their intentions and work with them to determine if we're heading down the right path. I appreciate that an issue was opened to start this discussion, but I think it's important we see it through before charging forth with whatever approach we deem convenient for our purposes.

If you're finding applying the event conventions is too nebulous a challenge at this point in time. I have some other ideas for moving forward.

1. Tell users to use a log processor to add the attributes.

   • This has the obvious downside that it's another component to configure and probably a bummer for performance on the logging hot path.

2. Offer experimental environment variables that enable a user to configure the attribute key themselves. Something like:

   • OTEL_DOTNET_EXPERIMENTAL_EXPORT_LOGRECORD_EVENT_ID_AS
   • OTEL_DOTNET_EXPERIMENTAL_EXPORT_LOGRECORD_EVENT_NAME_AS

These are questions I'm looking to you to answer. As the person recommending we use the event conventions, I'm leaning on you to describe how they're a good match for what you're trying to solve.

Regarding the event.domain - I am open to using this attribute after understanding the use case for this field. How does setting event.domain to ilogger or any other value helps users?

These are questions I'm looking to you to answer. As the person recommending we use the event conventions, I'm leaning on you to describe how they're a good match for what you're trying to solve. We can't go into this ignoring the intentions of the folk who designed the conventions. We need to strive to understand their intentions and work with them to determine if we're heading down the right path. I appreciate that an issue was opened to start this discussion, but I think it's important we see it through before charging forth with whatever approach we deem convenient for our purposes.

If you're finding applying the event conventions is too nebulous a challenge at this point in time. I have some other ideas for moving forward.

1. Tell users to use a log processor to add the attributes.

   • This has the obvious downside that it's another component to configure and probably a bummer for performance on the logging hot path.

2. Offer experimental environment variables that enable a user to configure the attribute key themselves. Something like:

   • OTEL_DOTNET_EXPERIMENTAL_EXPORT_LOGRECORD_EVENT_ID_AS
   • OTEL_DOTNET_EXPERIMENTAL_EXPORT_LOGRECORD_EVENT_NAME_AS

I am not recommending to use events conventions as is. If I was, I wouldn't put event.id in here. But let me clarify that I am not suggesting we ignore the spec entirely. That is why working with spec in parallel is imperative here.

I see this as an opportunity to bring multiple languages in sync and not define our own conventions. It's not just .NET that needs event.id. Moreover, this will help us get some feedback which we can share with specification. The conventions are experimental and we are going to offer them as experimental. IMO it is ok to deviate slightly. I would like to emphasize that the plan is to not ship these stable until we resolve these differences at spec level.

And to add, we have not stopped for spec to ship stable things in the past so, I would like to understand the rationale behind blocking changes that depends on experimental spec and experimental feature flag.

Additionally, I don't think we should be doing any workarounds. Setting experimental feature flag itself is workaround we are asking users to do on a stable library.

[vishweshbankwar]

In favor of making progress and unblocking this, I have updated the attributes to be logrecord.event.id and logrecord.event.name. Regarding the category name, have opened #4941 to export that as InstrumentationScope name under ScopeLogs.

@open-telemetry/dotnet-approvers @open-telemetry/dotnet-maintainers Please review.

[CodeBlanch]

LGTM