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

Flow the IMeterFactory to SocketsHttpHandler on Mobile #90298

Merged
merged 1 commit into from
Aug 10, 2023

Conversation

MihaZupan
Copy link
Member

I realized that what was actually happening here is that we weren't passing the IMeterFactory to the SocketsHttpHandler in our HttpClientHandler.AnyMobile.cs implementation.

You would end up with 2 separate meters:

  • one using the correct factory but not instrumenting the SocketsHttpHander
  • another one in SocketsHttpHandler that didn't have a factory set

The test that only listened to the metrics from one factory wouldn't see the SocketsHttpHander metrics, which is why I thought we weren't using the correct handler in #90225.

@MihaZupan MihaZupan added this to the 8.0.0 milestone Aug 10, 2023
@MihaZupan MihaZupan requested a review from a team August 10, 2023 07:32
@MihaZupan MihaZupan self-assigned this Aug 10, 2023
@ghost
Copy link

ghost commented Aug 10, 2023

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

Issue Details

I realized that what was actually happening here is that we weren't passing the IMeterFactory to the SocketsHttpHandler in our HttpClientHandler.AnyMobile.cs implementation.

You would end up with 2 separate meters:

  • one using the correct factory but not instrumenting the SocketsHttpHander
  • another one in SocketsHttpHandler that didn't have a factory set

The test that only listened to the metrics from one factory wouldn't see the SocketsHttpHander metrics, which is why I thought we weren't using the correct handler in #90225.

Author: MihaZupan
Assignees: MihaZupan
Labels:

area-System.Net.Http

Milestone: 8.0.0

@MihaZupan
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MihaZupan
Copy link
Member Author

Not seeing the failure mentioned in #90225 (comment) on Android runs anymore, so this seems to do the trick.

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM

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

Successfully merging this pull request may close these issues.

None yet

2 participants