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

Add logging sampling #5574

Draft
wants to merge 46 commits into
base: main
Choose a base branch
from
Draft

Add logging sampling #5574

wants to merge 46 commits into from

Conversation

evgenyfedorov2
Copy link
Contributor

@evgenyfedorov2 evgenyfedorov2 commented Oct 25, 2024 •
edited
Loading

Related to the #5123 proposal, this PR is focused on the logging sampling only. The buffering part will follow in a separate PR soon.

Microsoft Reviewers: Open in CodeFlow

noahfalk
noahfalk reviewed Oct 29, 2024
View reviewed changes
src/Libraries/Microsoft.Extensions.Telemetry.Abstractions/Sampling/SamplingParameters.cs Show resolved Hide resolved
src/Libraries/Microsoft.Extensions.Telemetry/Logging/ExtendedLogger.cs Outdated Show resolved Hide resolved
src/Libraries/Microsoft.Extensions.Telemetry.Abstractions/Sampling/SamplingParameters.cs Outdated Show resolved Hide resolved
src/Libraries/Microsoft.Extensions.Telemetry/Logging/ExtendedLoggerFactory.cs Outdated Show resolved Hide resolved
src/Libraries/Microsoft.Extensions.Telemetry/Sampling/SamplingLoggerBuilderExtensions.cs Outdated Show resolved Hide resolved
src/Libraries/Microsoft.Extensions.Telemetry/Sampling/TraceBasedSampler.cs Outdated Show resolved Hide resolved
src/Libraries/Microsoft.Extensions.Telemetry.Abstractions/Sampling/SamplingParameters.cs Outdated Show resolved Hide resolved
src/Libraries/Microsoft.Extensions.Telemetry/Sampling/RatioBasedSampler.cs Outdated Show resolved Hide resolved
src/Libraries/Microsoft.Extensions.Telemetry/Sampling/RatioBasedSampler.cs Outdated Show resolved Hide resolved
src/Libraries/Microsoft.Extensions.Telemetry/Sampling/SamplingLoggerBuilderExtensions.cs Outdated Show resolved Hide resolved
@noahfalk
Copy link
Member

@tarekgh - not sure if you have seen this yet?

@tarekgh
Copy link
Member

tarekgh commented Oct 30, 2024

Reviewed, in general, looks good. I added a minor question comments.

@RussKie RussKie requested a review from a team November 5, 2024 01:02
@evgenyfedorov2
Copy link
Contributor Author

In my doc, I had separated the idea of matching from what you do once you match. In this PR, these two things have been coupled as captured in the SamplingParameters struct.

The reason I think we need to separate "matching" from "actions upon matching" is that we have multiple actions possible. Once a record matches, we want to globally filter it, globally buffer it, filter it at the request level, and buffer it at the request level.

Discussed offline. Added configuration support allowing for specifying matching conditions per action. The action is only one for now - Ratio based sampler itself.

noahfalk
noahfalk reviewed Nov 7, 2024
View reviewed changes
src/Libraries/Microsoft.Extensions.Telemetry.Abstractions/Sampling/ILoggerSamplerFilterRule.cs Outdated Show resolved Hide resolved
src/Libraries/Microsoft.Extensions.Telemetry.Abstractions/Sampling/ILoggerSamplerFilterRule.cs Outdated Show resolved Hide resolved
src/Libraries/Microsoft.Extensions.Telemetry/Logging/ExtendedLogger.cs Outdated Show resolved Hide resolved
src/Libraries/Microsoft.Extensions.Telemetry/Sampling/RatioBasedSamplerFilterRule.cs Outdated Show resolved Hide resolved
src/Libraries/Microsoft.Extensions.Telemetry/Sampling/RatioBasedSamplerOptions.cs Outdated
/// Gets or sets the collection of <see cref="RatioBasedSamplerFilterRule"/> used for filtering log messages.
/// </summary>
#pragma warning disable CA1002 // Do not expose generic lists - List is necessary to be able to call .AddRange()
#pragma warning disable CA2227 // Collection properties should be read only - setter is necessary for options pattern
Copy link
Member

Choose a reason for hiding this comment

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

I assume its necessary if the implementation calls section.Get<RatioBasedSamplerOptions>(), but it wouldn't be necessary with a more manual implementation of the config parsing. I don't know how much this matters but if the API review folks wanted this not to be settable we could do it.

src/Libraries/Microsoft.Extensions.Telemetry/Sampling/SamplerRuleSelector.cs Outdated Show resolved Hide resolved
src/Libraries/Microsoft.Extensions.Telemetry/Sampling/SamplingLoggerBuilderExtensions.cs Outdated Show resolved Hide resolved
src/Libraries/Microsoft.Extensions.Telemetry/Sampling/SamplingLoggerBuilderExtensions.cs Outdated Show resolved Hide resolved
src/Libraries/Microsoft.Extensions.Telemetry/Sampling/RatioBasedSampler.cs Outdated
probability = 0.0;

// TO DO: check if we can optimize this. It is a hot path and
// we should be able to minimize number of rule selections on every log record.
Copy link
Member

Choose a reason for hiding this comment

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

I agree we'd want to optimize it but it doesn't impact the design review much. I think its good you left it as a TODO 👍.

@evgenyfedorov2 evgenyfedorov2 mentioned this pull request Nov 13, 2024
Draft
@dotnet-comment-bot
Copy link
Collaborator

‼️ Found issues ‼️

Project Coverage Type Expected Actual
Microsoft.Extensions.Telemetry Line 93 91.43 🔻
Microsoft.Extensions.Telemetry Branch 93 91.64 🔻

🎉 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.AzureAIInference 83 91

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

@dotnet-comment-bot
Copy link
Collaborator

‼️ Found issues ‼️

Project Coverage Type Expected Actual
Microsoft.Extensions.Telemetry Line 93 92.79 🔻

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

Project Expected Actual
Microsoft.Extensions.Diagnostics.Probes 70 76
Microsoft.Extensions.AI.AzureAIInference 83 91
Microsoft.Extensions.Caching.Hybrid 75 86

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

@dotnet-comment-bot
Copy link
Collaborator

‼️ Found issues ‼️

Project Coverage Type Expected Actual
Microsoft.Extensions.Telemetry Line 93 92.86 🔻
Microsoft.Extensions.Telemetry.Abstractions Line 100 99.72 🔻

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

Project Expected Actual
Microsoft.Extensions.AI.AzureAIInference 83 91
Microsoft.Extensions.Diagnostics.Probes 70 76
Microsoft.Extensions.Caching.Hybrid 75 78

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samsp-msft samsp-msft samsp-msft left review comments

@tarekgh tarekgh tarekgh left review comments

@noahfalk noahfalk noahfalk left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@evgenyfedorov2 evgenyfedorov2

Labels
area-resourcemonitoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@evgenyfedorov2 @geeknoid @noahfalk @tarekgh @dotnet-comment-bot @samsp-msft @RussKie