Skip to content

new System.Net.Http.EnableMetrics switch #113528

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

Closed

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Mar 14, 2025

Motivation: in browser/wasm the diagnostics and metrics are usually not usable in production deployment. They are making IL donwload bigger.

  • new switch System.Net.Http.EnableMetrics and use it in HTTP client to make MetricsHandler optional

TODO

  • unit test with metrics disabled
  • fix settings._metrics usage
  • make both System.Net.Http.EnableMetrics and System.Net.Http.EnableActivityPropagation opt-in for browser
  • make it into linker switch, so that it could be trimmed out

@pavelsavara pavelsavara added this to the 10.0.0 milestone Mar 14, 2025
@pavelsavara pavelsavara requested a review from MihaZupan March 14, 2025 12:52
@pavelsavara pavelsavara self-assigned this Mar 14, 2025
Copy link
Contributor

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

@filipnavara
Copy link
Member

Is there a reason not to reuse the existing MetricsSupport (.csproj) / System.Diagnostics.Metrics.Meter.IsSupported (AppConfig) switch?

@pavelsavara
Copy link
Member Author

Is there a reason not to reuse the existing MetricsSupport (.csproj) / System.Diagnostics.Metrics.Meter.IsSupported (AppConfig) switch?

Good question, I didn't think it thru yet. I just followed System.Net.Http.EnableActivityPropagation, which I guess gives different granularity.

@antonfirsov
Copy link
Member

antonfirsov commented Mar 14, 2025

I just followed System.Net.Http.EnableActivityPropagation, which I guess gives different granularity.

That design might be problematic. I don't know if it noticably hurts IL size or perf on wasm, mobile etc., but note that with #103922 we are now uncondtionally instantiating ActivitySource in multiple System.Net libraries, e.g.:

private static readonly ActivitySource s_activitySource = new ActivitySource(ActivitySourceName);

Note that both Metrics and Distributed Tracing have high-granularity opt-in mechanisms implemented by their listner infra. For IL size reduction IMO a global switch is a better solution.

@pavelsavara
Copy link
Member Author

I think we should quantify what is the extra size of IL or AOT image before we dive too deep.
I think the usage here would prevent Diagnostic types from IL trimming.

with #103922 we are now unconditionally instantiating ActivitySource in multiple System.Net libraries, e.g.:

Most of them are not supported in browser anyway. Except HTTP.

For IL size reduction IMO a global switch is a better solution.

Makes sense.

@steveisok is trimming this interesting for mobile OSes ?

@steveisok
Copy link
Member

@steveisok is trimming this interesting for mobile OSes ?

To date, android disables via MetricsSupport=false as it adds unnecessary startup cost. iOS hasn't had the need until now, but will do the same. I'd just bury that in the wasm SDK.

@pavelsavara pavelsavara closed this Apr 7, 2025
@pavelsavara
Copy link
Member Author

another approach in #114326

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

Successfully merging this pull request may close these issues.

4 participants