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

Remove InstrumentRecorder class #87873

Merged
merged 1 commit into from
Jun 21, 2023
Merged

Conversation

tarekgh
Copy link
Member

@tarekgh tarekgh commented Jun 21, 2023

Removing InstrumentRecorder from the runtime. MetricCollector can be used instead.

@ghost ghost assigned tarekgh Jun 21, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners new-api-needs-documentation labels Jun 21, 2023
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@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 Jun 21, 2023
@tarekgh tarekgh added this to the 8.0.0 milestone Jun 21, 2023
@tarekgh
Copy link
Member Author

tarekgh commented Jun 21, 2023

Copy link
Member

@joperezr joperezr left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks Tarek.

InstrumentRecorder<int> recorder = new InstrumentRecorder<int>(counter);

using MeterListener listener = new MeterListener();
listener.InstrumentPublished = (instrument, theListener) =>
Copy link
Member

Choose a reason for hiding this comment

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

This works and I'm fine if you merge it this way, but a simpler option is:

using MeterListener listener = new MeterListener();
int lastMeasurement = 0;
listener.SetMeasurementEventCallback<int>((inst, measurement, tags, state) => lastMeasurement = measurement);
listener.EnableMeasurementEvents(counter, null);

There is no requirement to use publishing events if you already know the counter you want to listen to.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll fix this in another PR. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

opened the PR #87898

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.

LGTM since InstrumentRecorder was added in P5 it's OK to remove now. You may want to issue a breaking change notice, just in case any customer's started using it in the preview.

@tarekgh
Copy link
Member Author

tarekgh commented Jun 21, 2023

You may want to issue a breaking change notice, just in case any customer's started using it in the preview.

I can add something to dotnet/core#8437

@tarekgh tarekgh mentioned this pull request Jun 22, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 22, 2023
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.

5 participants