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.

Sign up for GitHub

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

Jump to bottom

Publish UpDownCounter and ObservableUpDownCounter Events for System.Diagnostics.Metrics #81041

Merged
merged 3 commits into from
Jan 29, 2023
Merged

Publish UpDownCounter and ObservableUpDownCounter Events for System.Diagnostics.Metrics #81041

merged 3 commits into from
Jan 29, 2023

Conversation

kkeirstead
Copy link
Member

Publishing events for UpDownCounter and ObservableUpDownCounter for consumption by dotnet counters/dotnet monitor - this was likely omitted unintentionally in #64365. Includes testing changes for both instruments.

Fixes #79916

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jan 23, 2023
noahfalk
noahfalk approved these changes Jan 26, 2023
View reviewed changes
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.

Thanks @kkeirstead, this looks great. One request, can you change at least one of the MetricsEventSource tests to verify that the emitted data is correct when the UpDownCounter/ObservableUpDownCounter has negative rates and negative values. I don't predict any issue, but since that is the key difference of the UpDownCounter its good to confirm.

@build-analysis build-analysis bot mentioned this pull request Jan 26, 2023
Closed
noahfalk
noahfalk approved these changes Jan 26, 2023
View reviewed changes
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.

👍

@noahfalk
Copy link
Member

Are you happy with the changes at this point? I can sort out the test failures and get it merged if so. The failures appear unrelated to your change.

@kkeirstead
Copy link
Member Author

Are you happy with the changes at this point? I can sort out the test failures and get it merged if so. The failures appear unrelated to your change.

I think so - I didn't have any more changes I planned on making.

@noahfalk noahfalk merged commit dfe1076 into dotnet:main Jan 29, 2023
@noahfalk
Copy link
Member

Alright its in, thanks @kkeirstead!

@ghost ghost locked as resolved and limited conversation to collaborators Feb 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@noahfalk noahfalk noahfalk approved these changes

Assignees
No one assigned
Labels
area-System.Diagnostics.Metric community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

MetricsEventSource Not Publishing UpDownCounter For dotnet-counters/dotnet-monitor?
2 participants
@kkeirstead @noahfalk