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

Add Serilog example #107

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

TGrannen
Copy link

@TGrannen TGrannen commented Feb 4, 2024

Example for logging with Serilog #106

@TGrannen
Copy link
Author

TGrannen commented Feb 4, 2024

@dotnet-policy-service agree

Comment on lines 6 to 15
builder.Services.AddSerilog((_, loggerConfiguration) =>
{
// Configure Serilog as desired here for AppHost logs (or use IConfiguration)
loggerConfiguration
.ReadFrom.Configuration(builder.Configuration)
.MinimumLevel.Information()
.MinimumLevel.Override("Aspire.Hosting.Dcp", LogEventLevel.Warning)
.Enrich.FromLogContext()
.WriteTo.Console();
});
Copy link
Member

Choose a reason for hiding this comment

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

Though I've seen 1 person ask. I'm not sure what the point of pushing logs the AppHost logs through serilog.

Suggested change
builder.Services.AddSerilog((_, loggerConfiguration) =>
{
// Configure Serilog as desired here for AppHost logs (or use IConfiguration)
loggerConfiguration
.ReadFrom.Configuration(builder.Configuration)
.MinimumLevel.Information()
.MinimumLevel.Override("Aspire.Hosting.Dcp", LogEventLevel.Warning)
.Enrich.FromLogContext()
.WriteTo.Console();
});

Copy link
Author

Choose a reason for hiding this comment

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

Ya, I also saw it in one of the comments and figured I'd include it. But I'm not really sure why it would be super useful since the AppHost is really just wiring everything up.

I also included this block in the Readme if it makes more sense to just have it as a reference.

Comment on lines +13 to +14
/// ⚠ This method MUST be called before the <see cref="OpenTelemetryLoggingExtensions.AddOpenTelemetry(ILoggingBuilder)"/> method to still send structured logs via OpenTelemetry. ⚠
/// </remarks>
Copy link
Member

Choose a reason for hiding this comment

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

Why not disable the otlp logger in service defaults with a comment?

Copy link
Author

Choose a reason for hiding this comment

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

The oltp logger still needs to be enabled and configured to get Structured Logs to work in the dashboard (instead of just console logs). I attempted to use the Serilog OpenTelemetry Sink instead of the oltp logger, but I ran into the issue mentioned earlier today that ended up creating multiple records in the dashboard. It would be much better to switch to Serilog entirely, but I didn't know exactly what OpenTelemetry/Sink configuration might need to be added to prevent the issue in the dashboard. I've only lightly dabbled with OpenTelemetry so far and didn't have time to get too far into it on a Saturday night.

If there's a simple fix for the issue or somewhere that I could go to to get pointed down the right path, I'd be happy to try a few more things and rework this to switch everything over to just use Serilog.

I also mentioned this same thing in my comment on the issue related to this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Is this PR trying to showing using serilog alongside the otel logger provider?

Copy link
Author

Choose a reason for hiding this comment

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

That's correct, but more as a work around than what the final desired approach would be.

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.

2 participants