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

Jump to bottom

[otlp] Add log exception attributes under feature flag #4892

Merged
merged 21 commits into from
Oct 3, 2023
Merged

[otlp] Add log exception attributes under feature flag #4892

merged 21 commits into from
Oct 3, 2023

Conversation

vishweshbankwar
Copy link
Member

@vishweshbankwar vishweshbankwar commented Sep 26, 2023 •
edited
Loading

Towards #4875
Design discussion issue #

Changes

Adds an experimental feature flag OTEL_DOTNET_EXPERIMENTAL_EMIT_EXCEPTION_LOG_ATTRIBUTES to allow exporting attributes corresponding to LogRecord.Exception to otlp endpoint.

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
Copy link

codecov bot commented Sep 26, 2023 •
edited
Loading

Codecov Report

Merging #4892 (d17aa23) into main (b0038ae) will increase coverage by 0.18%.
The diff coverage is 96.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4892      +/-   ##
==========================================
+ Coverage   83.23%   83.41%   +0.18%     
==========================================
  Files         294      295       +1     
  Lines       12279    12294      +15     
==========================================
+ Hits        10220    10255      +35     
+ Misses       2059     2039      -20
Flag Coverage Δ
unittests 83.41% <96.15%> (+0.18%) ⬆️

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%> (ø)
....Exporter.OpenTelemetryProtocol/OtlpLogExporter.cs 100.00% <100.00%> (+6.25%) ⬆️
...rotocol/Implementation/OtlpLogRecordTransformer.cs 87.34% <94.11%> (ø)

... and 7 files with indirect coverage changes

@vishweshbankwar vishweshbankwar marked this pull request as ready for review September 27, 2023 00:40
@vishweshbankwar vishweshbankwar requested a review from a team September 27, 2023 00:40
CodeBlanch
CodeBlanch reviewed Oct 2, 2023
View reviewed changes
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/CHANGELOG.md Outdated
Comment on lines 5 to 10
* Added ability to export attributes corresponding to `LogRecord.Exception` i.e.
`exception.type`, `exception.message` and `exception.stacktrace`. These
attributes will be exported when
`OTEL_DOTNET_OTLP_EMIT_EXCEPTION_LOG_ATTRIBUTES` environment variable will be
set to `true`.
([#4892](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4892))
Copy link
Member

Choose a reason for hiding this comment

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

We should move this below the other 2 "unreleased" entries right? I've been telling people recently to order things chronologically hopefully I'm not mistaken 🤣

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 do most recent one at the top
https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry/CHANGELOG.md#unreleased

Copy link
Member

Choose a reason for hiding this comment

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

I vote for 1) the latest release should be at the top, the oldest release should be at the bottom 2) within a release, changes are ordered based on when the PR got merged (the latest one should be at the bottom).

Example for 1) https://github.com/dotnet/core/tree/main/release-notes/8.0#releases

Example for 2) https://github.com/dotnet/core/blob/main/release-notes/8.0/preview/8.0.0-rc.1.md#notable-fixes

Copy link
Member

Choose a reason for hiding this comment

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

@vishweshbankwar The unreleased section is at the top, yes, but the new entry goes into the bottom of that section 😄

Copy link
Member Author

@vishweshbankwar vishweshbankwar Oct 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I need to get my eyes checked😆 . Fixed the sequence.

CodeBlanch
CodeBlanch approved these changes Oct 2, 2023
View reviewed changes
Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

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

Sent a few nits but LGTM

@CodeBlanch CodeBlanch added the pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package label Oct 2, 2023
@CodeBlanch CodeBlanch merged commit c18ff65 into open-telemetry:main Oct 3, 2023
65 checks passed
@CodeBlanch CodeBlanch mentioned this pull request Oct 3, 2023
Merged
4 tasks
@JamesNK JamesNK mentioned this pull request Oct 11, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CodeBlanch CodeBlanch CodeBlanch approved these changes

@cijothomas cijothomas cijothomas left review comments

@reyang reyang reyang approved these changes

@alanwest alanwest alanwest approved these changes

Assignees
No one assigned
Labels
pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vishweshbankwar @alanwest @cijothomas @reyang @CodeBlanch