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 note on ilogger scopes #4818

Merged
merged 14 commits into from
Aug 30, 2023
Merged

[otlp] Add note on ilogger scopes #4818

merged 14 commits into from
Aug 30, 2023

Conversation

vishweshbankwar
Copy link
Member

@vishweshbankwar vishweshbankwar commented Aug 30, 2023 •
edited
Loading

Changes

Additional note on supporting ILogger scopes and special casing for attributes.

@vishweshbankwar vishweshbankwar changed the title [otlp] Add notes about experimental ilogger attributes and scopes [otlp] Add notes about experimental feature and ilogger scopes Aug 30, 2023
@codecov
Copy link

codecov bot commented Aug 30, 2023 •
edited
Loading

Codecov Report

Merging #4818 (1a20128) into main (1da0297) will decrease coverage by 0.01%.
Report is 1 commits behind head on main.
The diff coverage is n/a.

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

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4818      +/-   ##
==========================================
- Coverage   83.96%   83.95%   -0.01%     
==========================================
  Files         293      293              
  Lines       11966    11966              
==========================================
- Hits        10047    10046       -1     
- Misses       1919     1920       +1
Files Changed Coverage
...etryProtocol/Implementation/LogRecordExtensions.cs ø

@vishweshbankwar vishweshbankwar marked this pull request as ready for review August 30, 2023 20:57
@vishweshbankwar vishweshbankwar requested a review from a team August 30, 2023 20:57
vishweshbankwar
vishweshbankwar commented Aug 30, 2023
View reviewed changes
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/README.md Outdated
@@ -242,6 +246,28 @@ services.AddHttpClient(
Note: The single instance returned by `HttpClientFactory` is reused by all
export requests.

## Experimental features
Copy link
Member Author

Choose a reason for hiding this comment

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

@alanwest FYI I took this from your previous PR.

Copy link
Member

Choose a reason for hiding this comment

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

categorize this into signals (logs, metrics sub-headings)

reyang
reyang reviewed Aug 30, 2023
View reviewed changes
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/README.md Outdated
as attributes mapped as follows:
* `CategoryName` maps to `dotnet.ilogger.category_name`.
* `EventId` maps to two attributes `Id` and `Name` representing `Event.Id` and
`EventId.Name`, respectively.
Copy link
Member

Choose a reason for hiding this comment

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

Why is this EventId.Name instead of Event.Name (it seems super weird comparing with Event.Id)?

Copy link
Member Author

Choose a reason for hiding this comment

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

it is referring to https://learn.microsoft.com/en-us/dotnet/api/microsoft.extensions.logging.eventid?view=dotnet-plat-ext-7.0

Copy link
Member

@reyang reyang Aug 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I know but the names we have here sound wrong.

Copy link
Member Author

@vishweshbankwar vishweshbankwar Aug 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

There was a typo which I fixed. It should be EventId.Id not Event.Id. Sorry for the confusion

Copy link
Member

Choose a reason for hiding this comment

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

There was a typo which I fixed. It should be EventId.Id not Event.Id. Sorry for the confusion

It's getting even worse.

Copy link
Member Author

Choose a reason for hiding this comment

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

https://learn.microsoft.com/en-us/dotnet/api/microsoft.extensions.logging.eventid.id?view=dotnet-plat-ext-7.0

Copy link
Member

@cijothomas cijothomas Aug 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think @reyang is asking about the "attribute" key?
The attribute names are just "Id" and "Name" as per this doc.

CodeBlanch
CodeBlanch approved these changes Aug 30, 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.

LGTM

vishweshbankwar and others added 2 commits August 30, 2023 15:21
reyang
reyang requested changes Aug 30, 2023
View reviewed changes
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

Need to sort out the names before introducing the experimental feature.

@vishweshbankwar
Copy link
Member Author

Need to sort out the names before introducing the experimental feature.

I have removed the experimental part to revisit it as separate todo

@vishweshbankwar vishweshbankwar changed the title [otlp] Add notes about experimental feature and ilogger scopes [otlp] Add note on ilogger scopes Aug 30, 2023
utpilla
utpilla reviewed Aug 30, 2023
View reviewed changes
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/README.md
@@ -82,6 +82,15 @@ Core](../../examples/AspNetCore/Program.cs) example app. Check this
for configuring OpenTelemetry with `ILogger` for certain application types such
as ASP.NET Core and .NET Worker.

**ILogger Scopes**: OTLP Log Exporter supports exporting ILogger scopes as
Attributes. Scopes must be enabled at the SDK level using
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Attributes. Scopes must be enabled at the SDK level using
attributes. Scopes must be enabled at the SDK level using

@utpilla utpilla merged commit 72d0a6c into open-telemetry:main Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@utpilla utpilla utpilla left review comments

@CodeBlanch CodeBlanch CodeBlanch approved these changes

@reyang reyang reyang approved these changes

@cijothomas cijothomas cijothomas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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