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] Only initialize listeners once #90932

Merged
merged 3 commits into from
Aug 23, 2023

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Aug 22, 2023

Backport of #90902 to release/8.0 for RC2.

AddMetrics uses an options validation workaround to initialize the MetricsSubscriptionManager and IMetricsListeners. However, if AddMetrics is called multiple times, it will do the initialization multiple times. IMetricsListener implementations may not be expecting that.

Customer Impact

IMetricsListener implementations may not expect/handle Initialize being called multiple times.

Testing

Manual

Risk

Low, this was a new component in RC1.

@ghost
Copy link

ghost commented Aug 22, 2023

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

Issue Details

Backport of #90902 to release/8.0

/cc @Tratcher

Customer Impact

Testing

Risk

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.

Author: github-actions[bot]
Assignees: -
Labels:

area-Infrastructure-libraries

Milestone: -

@tarekgh
Copy link
Member

tarekgh commented Aug 22, 2023

@Tratcher could you please add a test for the issue fixed here? I mean a test calling AddMetrics multiple times.

@tarekgh
Copy link
Member

tarekgh commented Aug 22, 2023

@jeffhandley this issue needs your blessing to merge and track. Thanks!

@tarekgh tarekgh added the Servicing-consider Issue for next servicing release review label Aug 22, 2023
@tarekgh
Copy link
Member

tarekgh commented Aug 22, 2023

@Tratcher thanks for adding the test. When we get a green CI here, it will be good if you can backport this test to the main too.

Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

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

I approve of this fix for RC2. @Tratcher Did these tests get merged into main as well?

@Tratcher
Copy link
Member

The fix is in main. I'll merge the additional test into main after this.

@carlossanlop carlossanlop added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Aug 23, 2023
@carlossanlop carlossanlop merged commit 482bfb9 into release/8.0 Aug 23, 2023
103 of 108 checks passed
@carlossanlop carlossanlop deleted the backport/pr-90902-to-release/8.0 branch August 23, 2023 21:43
@Tratcher
Copy link
Member

/backport to main

@github-actions
Copy link
Contributor Author

Started backporting to main: https://github.com/dotnet/runtime/actions/runs/5956831413

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.

LGTM 👍

@ghost ghost locked as resolved and limited conversation to collaborators Sep 23, 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.

6 participants