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

Metrics APIs Additions #86567

Merged
merged 5 commits into from
May 23, 2023
Merged

Metrics APIs Additions #86567

merged 5 commits into from
May 23, 2023

Conversation

tarekgh
Copy link
Member

@tarekgh tarekgh commented May 22, 2023

Fixes #77514

@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.

@ghost ghost assigned tarekgh May 22, 2023
@tarekgh tarekgh added this to the 8.0.0 milestone May 22, 2023
@tarekgh tarekgh requested a review from noahfalk May 22, 2023 02:42
_cachedMeters.Add(options.Name, meterList);
}

Meter m = new Meter(options.Name, options.Version, options.Tags, scope: this);
Copy link
Member

Choose a reason for hiding this comment

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

Weren't we going to use a subclassed Meter who's exposed Dispose was a nop?

Copy link
Member Author

@tarekgh tarekgh May 22, 2023

Choose a reason for hiding this comment

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

I will investigate the disposal issue further to determine the best possible solution. I'll track the issue and will link it here.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. If we don't end up changing this, we need to change the name of the method. Calling it Create and returning something that's IDisposable but then telling people in docs not to Dispose it will lead to a pit of failure.

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 opened #86592

{
Name = name ?? throw new ArgumentNullException(nameof(name));
Version = version;
if (tags is not null)
{
Tags = new List<KeyValuePair<string, object?>>(tags);
Copy link
Member

Choose a reason for hiding this comment

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

The consumer could do:

((List<...>)meter.Tags).Add(...);

Would that mess anything up?

Copy link
Member Author

Choose a reason for hiding this comment

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

Users doing that will be missing up with themselves. It is not good to depend on the internal implementation anyway. But even if they do that in this case, it will not cause crashes or terrible things. Will just be updating a list that not supposed to be mutated.

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.

mostly looked good, a few comments inline


if (_isObservableInstrument)
{
_meterListener.RecordObservableInstruments();
Copy link
Member

Choose a reason for hiding this comment

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

Oh I missed this. At some point in the past I thought there was an explicit RecordObservableInstrument() API. Doing it this way feels weird to me because now GetMeasurements() is mutating the list every time it is called. I think the design would be better if we have a separate explicit API that records observable instruments. For example imagine a user wants to capture measurements at several points, then verify them:

//arrange
using recorder = new InstrumentRecorder<int>(observableInstrument);

//act
SetState1();
recorder.GetMeasurements();
SetState2();
recorder.GetMeasurements();
SetState3();
recorder.GetMeasurements();

//assert
//I want to confirm that all three cases produced a measurement.
//but I can't use GetMeasurements() here because it changes the count to 4.
Assert.Equals(3, recorder.GetMeasurements().Length);

Users could of course workaround it by saving a snapshot of the measurements earlier, but needing to treat the API differently for observable and non-observable instruments feels awkward. Having API called 'GetXXX' change the thing it is getting also seems like it wouldn't align with our naming rules.

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 keep whatever I have in this PR and then we can talk about this one to know if we really need a new API.

{
Name = name ?? throw new ArgumentNullException(nameof(name));
Version = version;
if (tags is not null)
{
Tags = new List<KeyValuePair<string, object?>>(tags);
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to use List rather than array? We should never modify this collection in the future. Also I'd suggest we sort it.

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 changed the List to Array. I'll address the tags sorting and tags insensitive comparisons in a separate PR when addressing #86614.

@tarekgh tarekgh merged commit da30389 into dotnet:main May 23, 2023
@ericstj ericstj added the blog-candidate Completed PRs that are candidate topics for blog post coverage label May 31, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Diagnostics.Metric blog-candidate Completed PRs that are candidate topics for blog post coverage new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API Proposal]: Introduce DI friendly IMeter<T> for modern services
8 participants