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/8.0] Metrics Feature Switch #92019

Merged
merged 4 commits into from
Sep 14, 2023

Conversation

tarekgh
Copy link
Member

@tarekgh tarekgh commented Sep 13, 2023

Fixes #89880

Backport of #91767 to release/8.0

/cc @tarekgh

Customer Impact

The introduction of a feature switch enables the option to disable the metrics feature, aiding in trimming and AOT scenarios. This modification addresses and resolves the performance regression observed in Xamarin app startup scenarios. For additional information, please refer to the following issue: #89880.

Testing

Successfully ran all regressions test and added a new test covering testing when the new switch is enabled.

Risk

We haven't made any alterations or adjustments to the code or logic when the new switch is left disabled. Therefore, any modifications will only become apparent when the switch is activated. The risk of this change adversely affecting any existing, functional functionality is exceedingly low.

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Sep 13, 2023
@ghost ghost assigned tarekgh Sep 13, 2023
@tarekgh tarekgh added area-System.Diagnostics.Metric and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Sep 13, 2023
@tarekgh tarekgh added this to the 8.0.0 milestone Sep 13, 2023
@tarekgh
Copy link
Member Author

tarekgh commented Sep 13, 2023

CC @noahfalk @eerhardt

@tarekgh tarekgh added the Servicing-consider Issue for next servicing release review label Sep 13, 2023
@tarekgh
Copy link
Member Author

tarekgh commented Sep 13, 2023

@ericstj please have a look and approve it if you don't see any concerns with it. Thanks!

Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

This LGTM and I approve for RC2.

One ask is to double check that this will integrate well by staging the changes you want to make in the SDK and confirming the right default / feature enabled behavior. I know I've mixed up defaults or true/false values more than once myself.

@ericstj
Copy link
Member

ericstj commented Sep 13, 2023

@artl93 this is ready for your approval as well.

@tarekgh
Copy link
Member Author

tarekgh commented Sep 14, 2023

One ask is to double check that this will integrate well by staging the changes you want to make in the SDK and confirming the right default / feature enabled behavior. I know I've mixed up defaults or true/false values more than once myself.

I am currently doing some more tests. Please hold merging till I finish. nothing alarming so far :-)

@tarekgh
Copy link
Member Author

tarekgh commented Sep 14, 2023

@ericstj I have done more manual testing building and running as AOT. I confirmed when the switch is not defined or it is defined with true value, I see the metrics functionality is enabled and work as expected. When defining the switch with false value, I am seeing the new behavior which disables metrics functionality. I think we are good to go. I haven't done the SDK changes yet which I'll do more testing when having it.

@artl93
Copy link
Member

artl93 commented Sep 14, 2023

@ericstj I have done more manual testing building and running as AOT. I confirmed when the switch is not defined or it is defined with true value, I see the metrics functionality is enabled and work as expected. When defining the switch with false value, I am seeing the new behavior which disables metrics functionality. I think we are good to go. I haven't done the SDK changes yet which I'll do more testing when having it.

For my education, what do you mean by, "When having it"? Do you mean after merge and the SDK is produced?

@ericstj
Copy link
Member

ericstj commented Sep 14, 2023

This change in dotnet/runtime will require corresponding change in the SDK targets to expose this new feature to the build. Those changes will look similar to these:
https://github.com/search?q=repo%3Adotnet%2Fsdk+EventSourceSupport+language%3AXML&type=code&l=XML

Copy link
Member

@artl93 artl93 left a comment

Choose a reason for hiding this comment

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

M2 approved. Thanks!

@artl93 artl93 added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 14, 2023
@carlossanlop carlossanlop merged commit 3aec961 into dotnet:release/8.0 Sep 14, 2023
108 of 109 checks passed
@radical radical mentioned this pull request Sep 26, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Oct 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants