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

Ensure MeterListener.RecordObservableInstruments invoking all instruments callbacks #56183

Conversation

tarekgh
Copy link
Member

@tarekgh tarekgh commented Jul 22, 2021

When having a MeterListener listening to multiple observable instruments and this listener is calling back all listening to instruments to collect instrumentation data; if one of the callbacks throw exception, we used to let this exception propagate. The remaining instruments callbacks will not get a chance to execute. This change is catching any exception thrown from the instrument callbacks and continue executing calling the remaining instruments. At the end of the operation, we'll throw AggregationException containing the list of all exceptions thrown during the operation.

@ghost
Copy link

ghost commented Jul 22, 2021

Tagging subscribers to this area: @tommcdon, @krwq
See info in area-owners.md if you want to be subscribed.

Issue Details

When having a MeterListener listening to multiple observable instruments and this listener is calling back all listening to instruments to collect instrumentation data; if one of the callbacks throw exception, we used to let this exception propagate. The remaining instruments callbacks will not get a chance to execute. This change is catching any exception thrown from the instrument callbacks and continue executing calling the remaining instruments. At the end of the operation, we'll throw AggregationException containing the list of all exceptions thrown during the operation.

Author: tarekgh
Assignees: -
Labels:

area-System.Diagnostics

Milestone: -

@tarekgh
Copy link
Member Author

tarekgh commented Jul 22, 2021

@noahfalk @cijothomas could you please have a look?

CC @shirhatti @reyang

@tarekgh tarekgh requested a review from noahfalk July 22, 2021 20:44
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! Once it is merged I can also confirm the errors are reported reasonably in dotnet-counters

cc @hoyosjs

Copy link
Contributor

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

LGTM.
Added a small suggestion for an additional test case

@tarekgh tarekgh force-pushed the CatchExceptionsInMetricsListenerAndThrowAggregateException branch from 573946b to 1100b8d Compare July 22, 2021 23:20
@tarekgh
Copy link
Member Author

tarekgh commented Jul 23, 2021

The CI failure is unrelated and tracked with the issue #56190

@tarekgh tarekgh merged commit c796482 into dotnet:main Jul 23, 2021
@tarekgh tarekgh deleted the CatchExceptionsInMetricsListenerAndThrowAggregateException branch July 23, 2021 16:33
@ghost ghost locked as resolved and limited conversation to collaborators Aug 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants