Skip to content

Add test for the order of activity and metrics with HTTP requests #56592

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

Merged
merged 2 commits into from
Jul 4, 2024

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Jul 3, 2024

Exemplars require metrics and activities are completed in the correct order. ASP.NET Core fortunately already does the right thing.

Add comments and a unit test to assert the correct behavior.

@JamesNK JamesNK added the area-hosting Includes Hosting label Jul 3, 2024
@JamesNK JamesNK requested a review from halter73 as a code owner July 3, 2024 12:08
@JamesNK
Copy link
Member Author

JamesNK commented Jul 3, 2024

cc @noahfalk @tarekgh @antonfirsov

A test that verifies an activity is active when a metric is recorded. This kind of test should be applied to HttpClient (it currently doesn't do the right thing) and anywhere else in the future that combines a duration histogram and activity.

Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

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

LGTM!

var hostingEventSource = new HostingEventSource(Guid.NewGuid().ToString());
using var activityListener = new ActivityListener
{
ShouldListenTo = activitySource => activitySource.Name == "Microsoft.AspNetCore",
Copy link
Member

Choose a reason for hiding this comment

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

You should create an activity source, inject it, and use reference equal so the test isn't affected by other tests.

See prior examples:

ShouldListenTo = activitySource => ReferenceEquals(activitySource, testSource),

Copy link
Member Author

@JamesNK JamesNK Jul 4, 2024

Choose a reason for hiding this comment

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

I don't think it matters for this test because it doesn't actually listen for activities and then use them. We're only listening so an activity is created.

But, for consistency and to reduce confusion, I'll update the test.

@JamesNK JamesNK force-pushed the jamesnk/metrics-activity-order-test branch from bb25c67 to 79ec511 Compare July 4, 2024 02:07
@JamesNK JamesNK enabled auto-merge (squash) July 4, 2024 02:09
@JamesNK JamesNK merged commit 8bf9970 into main Jul 4, 2024
26 checks passed
@JamesNK JamesNK deleted the jamesnk/metrics-activity-order-test branch July 4, 2024 04:14
@dotnet-policy-service dotnet-policy-service bot added this to the 9.0-preview7 milestone Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-hosting Includes Hosting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants