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 should enforce labels by typing #3151

Closed
tomerghelber-tm opened this issue Aug 7, 2022 · 3 comments · Fixed by #3170
Closed

Metrics should enforce labels by typing #3151

tomerghelber-tm opened this issue Aug 7, 2022 · 3 comments · Fixed by #3170
Labels
api:metrics Issues and PRs related to the Metrics API feature-request
Milestone

Comments

@tomerghelber-tm
Copy link
Contributor

tomerghelber-tm commented Aug 7, 2022

Is your feature request related to a problem? Please describe.

Right now, when a label is required for a Metric, it is not enforced and potentially corrupted metrics can be sent.
We are testing the output of the metrics but the promotheus library is able to enforce that.
It also enables us to put some constants as labels and create a wrapped metric, which needs fewer labels.

Describe the solution you'd like

Please add to the metrics a generic place for the required labels.
If it is possible to add the "partial labeling of a metric" it would be great!

Describe alternatives you've considered

We are wrapping the creation and access of the built-in metric to enforce the labels in the types.

Additional context

Here is some code for example:

class MyCounter<MissingLabelName extends string> {
    constructor(
        private readonly labelNames: MissingLabelName[],
        private readonly labelValues: Record<Exclude<string, MissingLabelName>, string>
    ) {
    }

    labels<FilledLabelName extends MissingLabelName>(
        labelValues: Record<FilledLabelName, string>
    ): MyCounter<Exclude<MissingLabelName, FilledLabelName>> {
        type StillMissingLabelName = Exclude<MissingLabelName, FilledLabelName>
        function isNotFilled(labelName: MissingLabelName): labelName is StillMissingLabelName {
            return !(labelName in labelValues)
        }
        const labelNames: StillMissingLabelName[] = this.labelNames.filter(isNotFilled)
        const filledLabelValues = {
            ...this.labelValues,
            ...labelValues
        } as unknown as Record<Exclude<string, Exclude<MissingLabelName, FilledLabelName>>, string>
        return new MyCounter<Exclude<MissingLabelName, FilledLabelName>>(
            labelNames,
            filledLabelValues
        );
    }

    public inc(
        value: number=1,
        labelValues: Record<MissingLabelName, string>
    ): void {
        const allLabelValue = {
            ...this.labelValues,
            ...labelValues
        } as Record<string, string>
        console.log(`${JSON.stringify(allLabelValue)} ${value}`)
    }
}

const counter = new MyCounter(["version", "type"])
const partial = counter.labels({ version: "1.0" })
partial.inc(2, { type: "double_click" })
counter.inc(1, { version: "1.1", type: "click" })
// partial.inc(1) TYPE ERROR
// counter.inc(1, { type: "double_click" }) TYPE ERROR
@legendecas
Copy link
Member

I think this is an interesting feature. I would suggest experimenting with the type interfaces in @opentelemetry/api-metrics and try not to put a requirement for SDKs to do anything to support it (i.e. enforcing the actual filters).

@dyladan
Copy link
Member

dyladan commented Aug 10, 2022

I think this is an interesting feature. I would suggest experimenting with the type interfaces in @opentelemetry/api-metrics and try not to put a requirement for SDKs to do anything to support it (i.e. enforcing the actual filters).

If the required labels are known by the user creating the metric then maybe this can be accomplished with generics on create{metricType} methods.

@tomerghelber-tm
Copy link
Contributor Author

Sorry, It was closed because I tried to merge in my fork

@legendecas legendecas added the api:metrics Issues and PRs related to the Metrics API label Sep 1, 2022
@dyladan dyladan added this to the Metrics GA milestone Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api:metrics Issues and PRs related to the Metrics API feature-request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants