Skip to content

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.

2 participants