-
Notifications
You must be signed in to change notification settings - Fork 772
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
[otlp] Remove ilogger and exception attributes #4781
[otlp] Remove ilogger and exception attributes #4781
Conversation
…hub.com/vishweshbankwar/opentelemetry-dotnet into vibankwa/remove-experimental-attributes
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4781 +/- ##
==========================================
- Coverage 85.46% 85.36% -0.10%
==========================================
Files 314 314
Lines 12940 12930 -10
==========================================
- Hits 11059 11038 -21
- Misses 1881 1892 +11
|
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/LogRecordExtensions.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. One small thing to address regarding a test.
test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpLogExporterTests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -2,6 +2,11 @@ | |||
|
|||
## Unreleased | |||
|
|||
* Excluded attributes corresponding to `LogRecord.EventId`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its better if we add a stronger message here, as this is a significant breaking change for users who are relying on these features.
Breaking/Please Note or something to catch attention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not having the EventId & Category properties makes it harder to filter log messages to the ones that you care about. They could go in the attributes as they are SDK specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not having the EventId & Category properties makes it harder to filter log messages to the ones that you care about. They could go in the attributes as they are SDK specific.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not having the EventId & Category properties makes it harder to filter log messages to the ones that you care about. They could go in the attributes as they are SDK specific.
Indeed. Missed this in last week's release notes and ran into this today. Was a real head-scratcher for a few hours because this is the last place I expected these (fairly ubiquitous) dotnet attrs to be getting dropped
Fixes #
Design discussion issue #
Changes
This PR excludes the following fields on
LogRecord
from being exported by the OTLP exporter:EventId
CategoryName
Exception
Please provide a brief description of the changes here.
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial changes