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

[release/5.0-rc2] Fix System.Net.Sockets telemetry #42726

Merged
merged 1 commit into from
Sep 25, 2020

Conversation

MihaZupan
Copy link
Member

Backport of #42188 to release/5.0-rc2
Fixes #41670

Customer Impact:

Bugs in new 5.0 telemetry (built mainly for YARP).

  • Sockets telemetry events are broken when Connect/Accept finishes asynchronously (which is majority of cases) -- the Stop event won't be correlated with Start event. Observers of the events won't be able to associate connection failures, or time it took to establish TCP connections -- important scenarios, e.g. for YARP.
  • NameResolution (DNS) telemetry events break following events of the HTTP request by making them appear as part of the NameResolution scope instead of following after it. It leads to cumbersome reading of following events, making end-to-end tracing hard to understand.

Testing

  • Added a significant amount of testing to ensure the proper events and counters are emitted as part of this change.
  • There are a lot of existing tests covering Sockets behavior in CI.
  • End-to-end manual validation in PerfView and using in-proc EventListener.

Risk

  • Small - There is a minimal impact on the non-Telemetry code path -- just moving part of code into a function. Majority of the changes are under IsTelemetryEnabled() check.
  • Telemetry code paths are brand new - added during 5.0 (end of June), not yet consumed by any customers (beyond potentially early experimentation)
  • if this change would introduce new problems, they would be limited to scenarios where telemetry is enabled.

@ghost
Copy link

ghost commented Sep 25, 2020

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

@MihaZupan
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MihaZupan
Copy link
Member Author

Test failures unrelated: #41606, #40798, #41953, #41929, #41531

@karelz
Copy link
Member

karelz commented Sep 25, 2020

We are ready for merge once approved by Tactics.

@danmoseley danmoseley added the Servicing-approved Approved for servicing release label Sep 25, 2020
@danmoseley danmoseley merged commit c5a3f49 into dotnet:release/5.0-rc2 Sep 25, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
@karelz karelz added this to the 5.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Sockets Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants