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

Make ResourceUtilizationInstruments class public #5345

Closed
evgenyfedorov2 opened this issue Aug 9, 2024 · 5 comments
Closed

Make ResourceUtilizationInstruments class public #5345

evgenyfedorov2 opened this issue Aug 9, 2024 · 5 comments
Assignees

Comments

@evgenyfedorov2
Copy link
Contributor

evgenyfedorov2 commented Aug 9, 2024

The Microsoft.Extensions.Diagnostics.ResourceMonitoring.ResourceUtilizationInstruments class contains only constants which represent metric names. Those metric names are used internally in the library, but they also might be used by customers in their code, and since metric names are internal, currently customers are forced to use hard-coded strings. Therefore, I propose to make the ResourceUtilizationInstruments class public, which will expose all its members (they are already public const).

Proposed API

-internal static class ResourceUtilizationInstruments
+public static class ResourceUtilizationInstruments
{
    public const string CpuUtilization = "process.cpu.utilization";
    public const string MemoryUtilization = "dotnet.process.memory.virtual.utilization";
}

Usage Examples

    InstrumentPublished = (instrument, meter) =>
    {
        if (instrument.Name == ResourceUtilizationInstruments.CpuUtilization)
        {
            meter.EnableMeasurementEvents(instrument, state: this);
        }
    }
@evgenyfedorov2 evgenyfedorov2 self-assigned this Aug 9, 2024
@evgenyfedorov2 evgenyfedorov2 added area-fundamentals enhancement This issue represents an ask for new feature or an enhancement to an existing one and removed untriaged labels Aug 9, 2024
@RussKie RussKie added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Aug 11, 2024
@RussKie
Copy link
Member

RussKie commented Aug 12, 2024

This proposal makes sense to me as otherwise consumers migrating from Microsoft.R9.Extensions.Diagnostics.ResourceUtilization to Microsoft.Extensions.Diagnostics.ResourceMonitoring will have to change their code and reference metric names explicitly (essentially hard-code those in their code). E.g., in a sample I had to do the following:
image

It'd be great if we could keep the original instrument names, however the underlying metric names have changed, so it doesn't make much sense.

Making this type public will also enable us to obsolete instruments, if that becomes necessary for one reason or another. That is, will be able to do something like this:

public static class ResourceUtilizationInstruments
{
+   [Obsolete("Use XYZ instrument instead", UrlFormat = "aka.ms/docs/...")]
    public const string CpuUtilization = "process.cpu.utilization";

    public const string MemoryUtilization = "dotnet.process.memory.virtual.utilization";
}

@joperezr @geeknoid @mwierzchowski: I'm inclinde to approve this proposal unless there are objections.

@RussKie RussKie removed the enhancement This issue represents an ask for new feature or an enhancement to an existing one label Aug 12, 2024
@joperezr
Copy link
Member

It makes me a bit nervous if we are the first ones to ever expose these constants in the sense of: Why hasn't this been done elsewhere with other meters? In general I'm not really opposed to it, but I'd be curious to hear what either @JamesNK or @noahfalk think about this given I don't think there is a lot of precedent of strong-typing meter names.

@JamesNK
Copy link
Member

JamesNK commented Aug 12, 2024

I don't know anywhere we do this for other metrics. Or telemetry in general. Meters, instruments, activity source names, activity names, tag names, event counters, etc.

If folks want to use telemetry in a .NET app, then they likely copy the name into a string from docs.

I don't think you should expose constants. It's possible to add it in the future if there is demand for it.

@noahfalk
Copy link
Member

noahfalk commented Aug 13, 2024

I feel the same as James. We haven't exposed these strings as public API constants for any other built-in telemetry and I've never seen any criticism/requests related to it.

Making this type public will also enable us to obsolete instruments, if that becomes necessary for one reason or another

Most references to the names of telemetry occur in config files or out-of-process tooling so I'd expect plenty of editing whether API constants are available or not.

@RussKie RussKie removed the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Aug 13, 2024
@RussKie
Copy link
Member

RussKie commented Aug 13, 2024

Thank you for your feedback. Closing as "rejected".

@RussKie RussKie closed this as completed Aug 13, 2024
@RussKie RussKie closed this as not planned Won't fix, can't repro, duplicate, stale Aug 15, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Sep 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants