Skip to content

Add logging buffering #5635

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
Merged
merged 55 commits into from
Apr 9, 2025
Merged

Add logging buffering #5635

merged 55 commits into from
Apr 9, 2025

Conversation

@evgenyfedorov2
Copy link
Member

@evgenyfedorov2 evgenyfedorov2 commented Nov 13, 2024 •
edited by dotnet-policy-service bot
Loading

Related to the #5123 proposal, this PR is focused on the logging buffering only. See also the sampling part #5574.

Microsoft Reviewers: Open in CodeFlow

noahfalk
noahfalk reviewed Nov 23, 2024
View reviewed changes
@evgenyfedorov2 evgenyfedorov2 force-pushed the evgenyfedorov2/log_buffering branch 2 times, most recently from e40b7b6 to 9d61d12 Compare December 12, 2024 16:30
@evgenyfedorov2 evgenyfedorov2 force-pushed the evgenyfedorov2/log_buffering branch from 9d61d12 to 2f1a335 Compare December 12, 2024 17:36
@evgenyfedorov2
Fix namespaces
f7eaab1 
Address PR comments
Add .NET 8 support
Add ExceptionJsonConverter
@evgenyfedorov2
Merge branch 'main' into evgenyfedorov2/log_buffering
1f464df
@evgenyfedorov2 evgenyfedorov2 force-pushed the evgenyfedorov2/log_buffering branch from 883334d to 022f00c Compare December 16, 2024 14:23
@evgenyfedorov2 evgenyfedorov2 force-pushed the evgenyfedorov2/log_buffering branch from 022f00c to a371d9c Compare December 16, 2024 14:51
@dotnet-comment-bot
Copy link
Collaborator

‼️ Found issues ‼️

Project Coverage Type Expected Actual
Microsoft.AspNetCore.Diagnostics.Middleware Line 100 96.03 🔻
Microsoft.AspNetCore.Diagnostics.Middleware Branch 100 98.51 🔻
Microsoft.Extensions.Telemetry Line 93 86.2 🔻
Microsoft.Extensions.Telemetry Branch 93 84.59 🔻
Microsoft.Extensions.Diagnostics.Testing Line 99 85.78 🔻
Microsoft.Extensions.Diagnostics.Testing Branch 99 79.57 🔻

🎉 Good job! The coverage increased 🎉
Update MinCodeCoverage in the project files.

Project Expected Actual
Microsoft.Extensions.Diagnostics.Probes 70 76
Microsoft.Extensions.Caching.Hybrid 75 86
Microsoft.Extensions.AI.OpenAI 72 77

Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=896358&view=codecoverage-tab

@evgenyfedorov2 evgenyfedorov2 force-pushed the evgenyfedorov2/log_buffering branch from e16928a to f6a2dec Compare March 26, 2025 16:29
@evgenyfedorov2 evgenyfedorov2 requested a review from geeknoid March 27, 2025 06:49
@evgenyfedorov2 evgenyfedorov2 force-pushed the evgenyfedorov2/log_buffering branch 4 times, most recently from f01cbff to edcd424 Compare March 27, 2025 16:24
@evgenyfedorov2 evgenyfedorov2 force-pushed the evgenyfedorov2/log_buffering branch from edcd424 to 6e92aec Compare March 27, 2025 17:10
@evgenyfedorov2 evgenyfedorov2 requested a review from geeknoid April 1, 2025 15:24
@evgenyfedorov2 evgenyfedorov2 enabled auto-merge (squash) April 2, 2025 13:18
@evgenyfedorov2
Copy link
Member Author

@dariusclay, could you please help review this on behalf of the telemetry area? Thanks!

@evgenyfedorov2
Copy link
Member Author

@RussKie could you review this for the parts related to dotnet/dotnet-extensions-infra parts, please?

RussKie
RussKie approved these changes Apr 8, 2025
View reviewed changes
Copy link
Contributor

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

Approving the infra side, but noting possible implementation issues.

@RussKie RussKie disabled auto-merge April 8, 2025 23:51
@RussKie RussKie added the waiting-author-feedback 📭 The author of this issue needs to respond in order for us to continue investigating this issue. label Apr 8, 2025
@dotnet-policy-service dotnet-policy-service bot removed the waiting-author-feedback 📭 The author of this issue needs to respond in order for us to continue investigating this issue. label Apr 9, 2025
@evgenyfedorov2 evgenyfedorov2 enabled auto-merge (squash) April 9, 2025 06:33
@evgenyfedorov2 evgenyfedorov2 merged commit 3ebeec1 into dotnet:main Apr 9, 2025
6 checks passed
@KalleOlaviNiemitalo KalleOlaviNiemitalo mentioned this pull request Apr 9, 2025
Closed
KalleOlaviNiemitalo
KalleOlaviNiemitalo reviewed Apr 9, 2025
View reviewed changes
src/Libraries/Microsoft.Extensions.Telemetry/Buffering/StringifyComprarer.cs

namespace Microsoft.Extensions.Diagnostics.Buffering;

internal sealed class StringifyComprarer : IEqualityComparer<KeyValuePair<string, object?>>
Copy link

Choose a reason for hiding this comment

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

This should have been StringifyComparer, not StringifyComprarer. It's not public though.

@github-actions github-actions bot locked and limited conversation to collaborators May 9, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@geeknoid geeknoid geeknoid approved these changes

@tarekgh tarekgh Awaiting requested review from tarekgh

@noahfalk noahfalk Awaiting requested review from noahfalk

@dariusclay dariusclay Awaiting requested review from dariusclay

+2 more reviewers

@KalleOlaviNiemitalo KalleOlaviNiemitalo KalleOlaviNiemitalo left review comments

@RussKie RussKie RussKie approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@evgenyfedorov2 evgenyfedorov2

Labels

area-telemetry

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@evgenyfedorov2 @dotnet-comment-bot @tarekgh @KalleOlaviNiemitalo @RussKie @noahfalk @geeknoid