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.

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

Remove ILogger scope from the ASP.NET Core logging getting started doc #5254

Merged
merged 1 commit into from
Jan 25, 2024

Conversation

reyang
Copy link
Member

@reyang reyang commented Jan 24, 2024

ILogger scope has introduced some confusion to folks who just want to get started.
This PR makes the getting-started doc simpler and cleaner.

@reyang reyang requested a review from a team January 24, 2024 22:21
Copy link

codecov bot commented Jan 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6250307) 83.38% compared to head (dea15b5) 83.18%.
Report is 25 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5254      +/-   ##
==========================================
- Coverage   83.38%   83.18%   -0.21%     
==========================================
  Files         297      271      -26     
  Lines       12531    11970     -561     
==========================================
- Hits        10449     9957     -492     
+ Misses       2082     2013      -69     
Flag Coverage Δ
unittests ?
unittests-Solution-Experimental 83.17% <100.00%> (?)
unittests-Solution-Stable 83.08% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...tation.AspNetCore/Implementation/HttpInListener.cs 89.72% <100.00%> (+0.14%) ⬆️
...AspNetCore/Implementation/HttpInMetricsListener.cs 89.47% <ø> (ø)
...plementation/HttpWebRequestActivitySource.netfx.cs 80.77% <100.00%> (ø)
...emetry/Metrics/Exemplar/SimpleExemplarReservoir.cs 80.43% <100.00%> (ø)

... and 34 files with indirect coverage changes

@CodeBlanch CodeBlanch merged commit 6f753f6 into open-telemetry:main Jan 25, 2024
29 checks passed
@noahfalk
Copy link

This looks fine to me.

fyi @davidfowl.

@davidfowl
Copy link
Contributor

Why did we remove that?

This is what the aspire default template has:

builder.Logging.AddOpenTelemetry(logging =>
{
    logging.IncludeFormattedMessage = true;
    logging.IncludeScopes = true;
});

@noahfalk
Copy link

noahfalk commented Jan 26, 2024

Because it appears to log a bunch of redundant data that OTel already knows how to capture. Do you believe there something important enough in there that it needs to be enabled by default?

#5239

@davidfowl
Copy link
Contributor

Do you believe there something important enough in there that it needs to be enabled by default?

Yes, it going to stay the default in aspire.

I don't see any duplication in the aspire dashboard (OTLP endpoint). Maybe its a specific kind of app?

@cijothomas
Copy link
Member

Here's the output from the getting-started app:

LogRecord.Timestamp:               2024-01-26T15:30:00.2604163Z
LogRecord.TraceId:                 7d63ba8a20f49c10982c966624c3bbf3
LogRecord.SpanId:                  fd093be1dff74bcf
LogRecord.TraceFlags:              None
LogRecord.CategoryName:            Program
LogRecord.Severity:                Info
LogRecord.SeverityText:            Information
LogRecord.Body:                    Food `{name}` price changed to `{price}`.
LogRecord.Attributes (Key:Value):
    name: artichoke
    price: 9.99
    OriginalFormat (a.k.a Body): Food `{name}` price changed to `{price}`.
LogRecord.EventId:                 344095174
LogRecord.EventName:               FoodPriceChanged
LogRecord.ScopeValues (Key:Value):
[Scope.0]:SpanId: fd093be1dff74bcf
[Scope.0]:TraceId: 7d63ba8a20f49c10982c966624c3bbf3
[Scope.0]:ParentId: 0000000000000000
[Scope.1]:ConnectionId: 0HN0UGEKB9KAE
[Scope.2]:RequestId: 0HN0UGEKB9KAE:00000001
[Scope.2]:RequestPath: /

Scopes are duplicating the spanid,traceid. And other attributes as well coming from asp.net components, which user may not want.

@davidfowl
Copy link
Contributor

Is this specific to how the console log exporter in OTEL outputs the scopes?

@reyang reyang deleted the reyang/remove-scope-from-doc branch January 26, 2024 17:14
@cijothomas
Copy link
Member

Is this specific to how the console log exporter in OTEL outputs the scopes?

No. OTLP Exporter would have the same. (Only thing specific to console exporter is it prefixes "scope-depth", which is not done by OTLP Exporter)

@davidfowl
Copy link
Contributor

I think this is fine to leave out in this sample but aspire is leaving it in 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants