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

vishweshbankwar · 2023-08-08T15:45:20Z

Fixes #4404
Design discussion issue #

Changes

This PR updates names that will be exported to otlp endpoint when user specifies EventId details on ILogger in favor of fixing #4404. This change will be available to users via feature flag until we resolve #4776 [Feature flag to be added as a separate PR]

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)

codecov · 2023-08-08T15:58:44Z

Codecov Report

Merging #4754 (2563cf6) into main (b0ab8d5) will not change coverage.
Report is 4 commits behind head on main.
The diff coverage is 100.00%.

❗ Current head 2563cf6 differs from pull request most recent head 54fc09e. Consider uploading reports for the commit 54fc09e to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4754   +/-   ##
=======================================
  Coverage   82.05%   82.05%           
=======================================
  Files         313      313           
  Lines       12742    12742           
=======================================
  Hits        10455    10455           
  Misses       2287     2287

Files Changed	Coverage Δ
...etryProtocol/Implementation/LogRecordExtensions.cs	`92.50% <100.00%> (ø)`

... and 5 files with indirect coverage changes

reyang · 2023-08-08T18:52:50Z

test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpLogExporterTests.cs

        Assert.Contains("10", otlpLogRecordAttributes);
-        Assert.Contains("Name", otlpLogRecordAttributes);
+        Assert.Contains("dotnet.ilogger.event_name", otlpLogRecordAttributes);


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

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

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.

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

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.

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.

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.

I support removing it from the exporter for stable release.

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:

For anything that's not covered by the spec or debatable - remove those features.

Ship stable version of OTLP Exporter.

Gather feedback, and based on the feedback, add experimental features via feature flagging or experimentation, take more feedback.

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

Once the feature got validated by the users and we also feel comfortable about the direction, ship it as stable (remove the flag).

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

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

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.

reyang

Is this a breaking change? What's the plan?

vishweshbankwar · 2023-08-08T18:59:23Z

Is this a breaking change? What's the plan?

It is not a breaking change. Otlp log exporter is still in preview. Edit: The plan is to stabilize these names before shipping stable version.

reyang · 2023-08-08T20:52:29Z

The plan is to stabilize these names

How do you plan to stabilize these?

vishweshbankwar · 2023-08-08T21:07:53Z

The plan is to stabilize these names

How do you plan to stabilize these?

Finalize is what I meant, The change in this PR is proposal for that.

vishweshbankwar · 2023-08-17T18:56:50Z

closing in favor of #4781

vishweshbankwar and others added 2 commits August 7, 2023 09:23

[Otlp] Rename ilogger event id and name attributes

9d6f318

Merge branch 'main' into vibankwa/update-ilogger-event-attribute-names

c107dc5

Merge branch 'main' into vibankwa/update-ilogger-event-attribute-names

2563cf6

vishweshbankwar marked this pull request as ready for review August 8, 2023 18:48

vishweshbankwar requested a review from a team August 8, 2023 18:48

reyang reviewed Aug 8, 2023

View reviewed changes

reyang requested changes Aug 8, 2023

View reviewed changes

CodeBlanch mentioned this pull request Aug 9, 2023

[repo] Cleanup unused using statements #4759

Merged

Merge branch 'main' into vibankwa/update-ilogger-event-attribute-names

54fc09e

alanwest mentioned this pull request Aug 11, 2023

OTLP Exporter: Conditionally compile experimental log features #4762

Closed

vishweshbankwar closed this Aug 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

vishweshbankwar commented Aug 8, 2023 •

edited

Loading

codecov bot commented Aug 8, 2023 •

edited

Loading

reyang Aug 8, 2023

vishweshbankwar Aug 8, 2023

cijothomas Aug 8, 2023

CodeBlanch Aug 8, 2023

alanwest Aug 8, 2023

reyang Aug 8, 2023

alanwest Aug 11, 2023

vishweshbankwar Aug 11, 2023

reyang Aug 11, 2023

vishweshbankwar Aug 16, 2023

reyang left a comment

vishweshbankwar commented Aug 8, 2023 •

edited

Loading

reyang commented Aug 8, 2023

vishweshbankwar commented Aug 8, 2023

vishweshbankwar commented Aug 17, 2023

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

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

Conversation

vishweshbankwar commented Aug 8, 2023 • edited Loading

Changes

Merge requirement checklist

codecov bot commented Aug 8, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reyang left a comment

Choose a reason for hiding this comment

vishweshbankwar commented Aug 8, 2023 • edited Loading

reyang commented Aug 8, 2023

vishweshbankwar commented Aug 8, 2023

vishweshbankwar commented Aug 17, 2023

vishweshbankwar commented Aug 8, 2023 •

edited

Loading

codecov bot commented Aug 8, 2023 •

edited

Loading

vishweshbankwar commented Aug 8, 2023 •

edited

Loading