Skip to content

Support independently enabling hosting event counters and metrics #50565

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 1 commit into from
Sep 7, 2023

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Sep 7, 2023

Addresses #50412

This allows hosting event counters and metrics to be independently enabled. If only metrics are enabled, then the event source Interlocked calls are skipped. However, there is no performance improvement if both event counters and metrics are enabled.

@ghost ghost added area-hosting Includes Hosting labels Sep 7, 2023
@adityamandaleeka
Copy link
Member

adityamandaleeka commented Sep 7, 2023

LGTM. Can you please try the crank command they're using and see what the results look like? @EgorBo posted it in the issue.

cc @JulieLeeMSFT

@adityamandaleeka
Copy link
Member

BTW this is approved for 8.0, pending confirmation from @JulieLeeMSFT or @EgorBo that it meets their needs.

@JamesNK
Copy link
Member Author

JamesNK commented Sep 7, 2023

LGTM. Can you please try the crank command they're using and see what the results look like? @EgorBo posted it in the issue.

If there an easy way to run that command on the PR changes?

@adityamandaleeka
Copy link
Member

Check out --[JOB].source.branchOrCommit here: https://github.com/dotnet/crank/blob/main/src/Microsoft.Crank.Controller/README.md

I think that may do it...

Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

👍

{
context.EventLogOrMetricsEnabled = true;
context.MetricsEnabled = true;
context.MetricsTagsFeature ??= new HttpMetricsTagsFeature();
Copy link
Member

Choose a reason for hiding this comment

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

This gets allocated per request when metrics are on?

Copy link
Member Author

Choose a reason for hiding this comment

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

It’s cached on the context

@JulieLeeMSFT
Copy link
Member

Thanks for the quick fix!!

@adityamandaleeka
Copy link
Member

@JamesNK Ready for merge?

@JamesNK
Copy link
Member Author

JamesNK commented Sep 7, 2023

Yes

@EgorBo
Copy link
Member

EgorBo commented Sep 7, 2023

Is it going to be backported to 8.0 ?

@JamesNK
Copy link
Member Author

JamesNK commented Sep 7, 2023

Is it going to be backported to 8.0 ?

Yes. This PR is on the release/8.0 branch. aspnetcore is automatically forward merging to main.

@adityamandaleeka adityamandaleeka merged commit 9e5b381 into release/8.0 Sep 7, 2023
@adityamandaleeka adityamandaleeka deleted the jamesnk/hosting-counters-enable branch September 7, 2023 23:07
@ghost ghost added this to the 8.0-rc2 milestone Sep 7, 2023
@EgorBo
Copy link
Member

EgorBo commented Sep 11, 2023

it seesm that it didn't help with the initial issue:

crank --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/scenarios/json.benchmarks.yml --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/build/azure.profile.yml --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/scenarios/steadystate.profile.yml --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/build/ci.profile.yml --scenario json --profile arm-lin-28-app --profile intel-load2-load --application.framework net8.0 --application.collectDependencies true --application.options.collectCounters true --application.options.counterProviders "Microsoft.AspNetCore.Hosting"

still 2x slow, it uses aspnet from this commit: a413f2e0f8ec (yesterday) so presumably includes this fix.
cc @JamesNK

@ghost
Copy link

ghost commented Sep 11, 2023

Hi @EgorBo. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@davidfowl
Copy link
Member

It wouldn't fix the issue because crank uses event counters and we did nothing to improve the usage of interlocked.

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.

6 participants