Skip to content

disable/trim MetricsHandler when System.Diagnostics.Metrics.Meter.IsSupported is false #114326

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 24 commits into from
Apr 15, 2025
Merged

disable/trim MetricsHandler when System.Diagnostics.Metrics.Meter.IsSupported is false #114326

merged 24 commits into from
Apr 15, 2025

Conversation

@pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Apr 7, 2025 •
edited
Loading

  • use System.Diagnostics.Metrics.Meter.IsSupported to disable/trim MetricsHandler
  • use System.Net.Http.EnableActivityPropagation to disable/trim DiagnosticsHandler

@pavelsavara
- rebase
4b531b1 
- use System.Diagnostics.Metrics.Meter.IsSupported
- call EventSource.EnableActivityTracker() when DiagnosticsHandler is enabled
@pavelsavara
more
979efe8
@pavelsavara pavelsavara added this to the 10.0.0 milestone Apr 7, 2025
@pavelsavara pavelsavara self-assigned this Apr 7, 2025
@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

@pavelsavara pavelsavara mentioned this pull request Apr 7, 2025
Closed
@ManickaP
Copy link
Member

ManickaP commented Apr 7, 2025

cc @antonfirsov

@pavelsavara pavelsavara mentioned this pull request Apr 7, 2025
Merged
@build-analysis build-analysis bot mentioned this pull request Apr 7, 2025
Open
@pavelsavara pavelsavara requested a review from antonfirsov April 7, 2025 14:40
This was referenced Apr 7, 2025
Open
Closed
Open
@pavelsavara pavelsavara marked this pull request as ready for review April 7, 2025 15:11
@Copilot Copilot AI review requested due to automatic review settings April 7, 2025 15:11
@pavelsavara pavelsavara requested a review from lewing as a code owner April 7, 2025 15:11
Copilot
Copilot AI reviewed Apr 7, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 11 out of 15 changed files in this pull request and generated 2 comments.

Files not reviewed (4)
  • src/libraries/System.Net.Http/src/ILLink/ILLink.Substitutions.xml: Language not supported
  • src/libraries/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj: Language not supported
  • src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Descriptors.EventSource.xml: Language not supported
  • src/mono/sample/wasm/browser-advanced/Wasm.Advanced.Sample.csproj: Language not supported

@build-analysis build-analysis bot mentioned this pull request Apr 7, 2025
Open
javiercn
javiercn approved these changes Apr 8, 2025
View reviewed changes
Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

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

LGTM!

@build-analysis build-analysis bot mentioned this pull request Apr 8, 2025
Open
3 tasks
antonfirsov
antonfirsov reviewed Apr 8, 2025
View reviewed changes
Copy link
Contributor

@antonfirsov antonfirsov left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Does the PR ensure that the SocketsHttpHandlerMetrics and the ConnectionMetrics types are also trimmed away when metrics are disabled?

This was referenced Apr 10, 2025
Open
Open
@pavelsavara
Copy link
Member Author

pavelsavara commented Apr 11, 2025 •
edited
Loading

I added tests. Trimming doesn't work. The constant is not being propagated. I don't know why.

Fixed. It works when there are no indirections between the trimmed property and methods that use them

@pavelsavara
Merge branch 'main' into browser_MetricsHandler_optional
89a764a 
# Conflicts:
#	src/libraries/System.Net.Http/src/System/Net/Http/GlobalHttpSettings.cs
@pavelsavara
fix
6797af3
@pavelsavara pavelsavara requested a review from antonfirsov April 11, 2025 09:32
@pavelsavara pavelsavara mentioned this pull request Apr 14, 2025
Closed
antonfirsov
antonfirsov approved these changes Apr 14, 2025
View reviewed changes
Copy link
Contributor

@antonfirsov antonfirsov left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM modulo small suggestion. Thanks!

Edit: Just noticed MetricsHandlerTrimmedTest.cs is failing, will check this again after the fix.

pavelsavara and others added 2 commits April 15, 2025 08:18
@pavelsavara @antonfirsov
Update src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpH…
900f62f 
…andler/ConnectionPool/HttpConnectionWaiter.cs

Co-authored-by: Anton Firszov <[email protected]>
@pavelsavara
fix
cbd6446
@build-analysis build-analysis bot mentioned this pull request Apr 15, 2025
Closed
@pavelsavara pavelsavara merged commit d0e6ce8 into dotnet:main Apr 15, 2025
141 of 151 checks passed
@pavelsavara pavelsavara deleted the browser_MetricsHandler_optional branch April 15, 2025 12:07
@antonfirsov antonfirsov mentioned this pull request Apr 24, 2025
Merged
@github-actions github-actions bot locked and limited conversation to collaborators May 16, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

Copilot code review Copilot Copilot left review comments

@javiercn javiercn javiercn approved these changes

@lewing lewing Awaiting requested review from lewing lewing is a code owner

@MihaZupan MihaZupan Awaiting requested review from MihaZupan

+1 more reviewer

@antonfirsov antonfirsov antonfirsov approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@pavelsavara pavelsavara

Labels

area-System.Net.Http

Projects

None yet

Milestone

10.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@pavelsavara @ManickaP @antonfirsov @javiercn @MihaZupan