Skip to content
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.

Sign up for GitHub

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

[Otlp] Rename ilogger event id and name attributes #4754

Closed
wants to merge 4 commits into from
Closed

[Otlp] Rename ilogger event id and name attributes #4754

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9d6f318
[Otlp] Rename ilogger event id and name attributes
vishweshbankwar Aug 7, 2023
c107dc5
Merge branch 'main' into vibankwa/update-ilogger-event-attribute-names
vishweshbankwar Aug 8, 2023
2563cf6
Merge branch 'main' into vibankwa/update-ilogger-event-attribute-names
vishweshbankwar Aug 8, 2023
54fc09e
Merge branch 'main' into vibankwa/update-ilogger-event-attribute-names
vishweshbankwar Aug 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/LogRecordExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,12 +95,12 @@ internal static OtlpLogs.LogRecord ToOtlpLog(this LogRecord logRecord, SdkLimitO

if (logRecord.EventId.Id != default)
{
otlpLogRecord.AddIntAttribute(nameof(logRecord.EventId.Id), logRecord.EventId.Id, attributeCountLimit);
otlpLogRecord.AddIntAttribute("dotnet.ilogger.event_id", logRecord.EventId.Id, attributeCountLimit);
}

if (!string.IsNullOrEmpty(logRecord.EventId.Name))
{
otlpLogRecord.AddStringAttribute(nameof(logRecord.EventId.Name), logRecord.EventId.Name, attributeValueLengthLimit, attributeCountLimit);
otlpLogRecord.AddStringAttribute("dotnet.ilogger.event_name", logRecord.EventId.Name, attributeValueLengthLimit, attributeCountLimit);
}

if (logRecord.Exception != null)
Expand Down
6 changes: 3 additions & 3 deletions test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpLogExporterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ public void CheckToOtlpLogRecordEventId()
var otlpLogRecordAttributes = otlpLogRecord.Attributes.ToString();

// Event
Assert.Contains("Id", otlpLogRecordAttributes);
Assert.Contains("dotnet.ilogger.event_id", otlpLogRecordAttributes);
Assert.Contains("10", otlpLogRecordAttributes);

logRecords.Clear();
Expand All @@ -282,9 +282,9 @@ public void CheckToOtlpLogRecordEventId()
otlpLogRecordAttributes = otlpLogRecord.Attributes.ToString();

// Event
Assert.Contains("Id", otlpLogRecordAttributes);
Assert.Contains("dotnet.ilogger.event_id", otlpLogRecordAttributes);
Assert.Contains("10", otlpLogRecordAttributes);
Assert.Contains("Name", otlpLogRecordAttributes);
Assert.Contains("dotnet.ilogger.event_name", otlpLogRecordAttributes);
Copy link
Member

Choose a reason for hiding this comment

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

Why not using "event.name"? https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/event-api.md#emit-event

Copy link
Member Author

Choose a reason for hiding this comment

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

The intention is to clearly show where this attribute is coming from hence the attribute has ilogger in it.

Copy link
Member

Choose a reason for hiding this comment

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

it'll be tricky to use "event.name" as that is coming from an experimental spec, and the intention is to GA the OTLP Exporter.

Copy link
Member

Choose a reason for hiding this comment

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

This is something @alanwest and I have chatted a lot about and a topic we have discussed on the log SIG meeting a few times. Basically, should things that look like events use the "Event API" as the spec defines it? For example, EventSource also has event name & id should those use the event semantic conventions? So far the thinking has been these are NOT events as the spec defines them, just things that happen to use similar terms, and they should use names tied to the logging framework emitting them (dotnet.ilogger.event_name, dotnet.eventsource.event_name, etc.) NOT the event semantic conventions (event.name). 🤷

Copy link
Member

Choose a reason for hiding this comment

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

Bear with me, taking a bit of a tangent here for a sec...

In our prior two SIG meetings, we discussed closing #3491 and leaving our handling of CategoryName in the OTLP exporter. I've more or less conceded to the opinions of others, however for the record I've voiced that I hate the idea of leaving this behavior in the OTLP exporter 😆.

I'm going to make one last ditch effort to reemphasize my original stance on #3491. That is: Abandon the use of CategoryName from all SDK components. In other words, the OTLP exporter will not in anyway export CategoryName. The only way for it to be exported would be for some component upstream to add ILogger's category as a proper attribute.

As this applies to this PR, I think we should abandon the use of EventId from all SDK components.

This has the benefit of not requiring us to settle on some dotnet.ilogger.* convention today. Mind you, settling on semantic conventions for logging frameworks should be a very carefully considered effort. I'd far prefer not prioritizing this effort right now in haste.

Copy link
Member

Choose a reason for hiding this comment

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

If everyone here is ok with these names, I would like to keep this PR as is and introduce the flag part in separate PR.

I'm not okay, I want to see discussions around the name, e.g. why would we want this vs. that, and the discussion should be backed by user scenarios.

Copy link
Member

Choose a reason for hiding this comment

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

When you say move this functionality out of the OTLP exporter, do you mean these attributes should be added in the SDK instead? If yes, I think there some perf concerns that were raised by @CodeBlanch regarding this.

Yes this is what I mean. Especially since in our SIG meeting discussions it seemed that folks really liked ilogger in the attribute names. ILogger specific instrumentation or nomenclature has no place in the OTLP exporter in my opinion.

I had suggested we consider a more general attribute names. Something like dotnet.logrecord.event_id. But to @reyang's point whatever we decide we need to carefully consider it and because of that we should not ship this hastily.

I'd have been fine feature flagging the names with ilogger in them, but if @reyang you feel that a deeper discussion about names is required even to ship them feature flagged then #4762 just removes the attributes altogether in our stable release. As I voiced earlier, I actually preferred removing them for now anyways.

Copy link
Member Author

Choose a reason for hiding this comment

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

I support removing it from the exporter for stable release.

Copy link
Member

Choose a reason for hiding this comment

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

I'd have been fine feature flagging the names with ilogger in them, but if @reyang you feel that a deeper discussion about names is required even to ship them feature flagged then #4762 just removes the attributes altogether in our stable release. As I voiced earlier, I actually preferred removing them for now anyways.

I don't feel we should spend more time on this topic right now, since the high order bit is to ship stable version. Agree with what you said @alanwest, here is the sequence in my mind:

  1. For anything that's not covered by the spec or debatable - remove those features.
  2. Ship stable version of OTLP Exporter.
  3. Gather feedback, and based on the feedback, add experimental features via feature flagging or experimentation, take more feedback.
  4. Once we got a good amount of feedback, come back to the discussion and decision making (we don't want to keep discussing something without new inputs/feedback).
  5. Once the feature got validated by the users and we also feel comfortable about the direction, ship it as stable (remove the flag).

Copy link
Member Author

Choose a reason for hiding this comment

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

We discussed this in the SIG meeting yesterday. Here is the plan:

  1. We will go ahead with this change in favor of resolving the issue faced by user in Logs exporter: flattening EventId into Id and Name causes avoidable attribute name conflicts #4404
  2. These attributes will only be added if user enables them via experimental feature flag (to be part of separate PR)

NOTE: The names and handling of these attributes are not to be treated as the final design and the discussion around it can continue on #4776.

Assert.Contains("MyEvent10", otlpLogRecordAttributes);
}

Expand Down