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

[WIP] Otlp: Add info metrics identifiers labels in open metrics specification #650

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ying-jeanne
Copy link
Contributor

@ying-jeanne ying-jeanne commented Jun 11, 2024

This is a prototype for addressing identifier labels in OpenMetric

open for discussion, See design doc https://docs.google.com/document/d/1UC6crn7WO-iKRxwRZEg5mjTyiqHk5WbIWfbnsYsI1xw/edit?usp=sharing

In order to support info metrics we need to flag identifier labels in the open metrics schema, this PR intent to implement option 4 of design doc and add identifier labels in the TYPE metadata line separate each labels by comma.

@ying-jeanne ying-jeanne requested review from beorn7 and aknuds1 June 11, 2024 22:34
@ying-jeanne ying-jeanne force-pushed the add-info-open-metrics branch from 36a7c10 to c415611 Compare June 11, 2024 22:55
@beorn7
Copy link
Contributor

beorn7 commented Jun 12, 2024

Was this discussed anywhere? Is this meant as a PoC to propose a change in OpenMetrics?

I think we should write down some thoughts before implementing anything (but having some PoC code is fine – that's why I'm asking if there is maybe some written down thoughts or a design doc anywhere).

My random thoughts:

  • OpenMetrics protobuf already has the distinction between identifying and data labels. There isn't any implementation using OM proto yet (to my knowledge), but we can use the proto structure as inspiration for how to design the text format and what the parser should yield. (E.g. I could imagine to not return the data labels at all as part of the usual labels-returning functions but have a separate call for them. In this PR, it looks like all labels will be returned, and there is a separate call for the identifying labels, which would be a very different approach.)
  • Adding another metadata line in OM text format is probably the way to go, but why IDENS as the keyword? Maybe just ID as the shortest recognizable keyword?
  • We need to specify a default behavior (if the ID line is missing). I guess that "all the labels are data labels" is a good default, and nicely matches an empty ID line.
  • Comma separation is problematic in light of the current plans to support all of UTF-8 in label names (as OTel does). We needed some quoting rules. Or maybe we can just require every identifying label to be on its own repeated ID line? I believe it will be rare to have an identifying label at all in the exposition (they usually come from target labels upon scrape), not to speak about having two or more. But I'm not sure if OM has a problem with repeated metadata lines.

@aknuds1
Copy link
Contributor

aknuds1 commented Jun 13, 2024

@beorn7 Ying, Jesus, and I just discussed - we think there should be a design doc for hashing out the change :) I figure @ying-jeanne will create it, and invite you to discuss.

@ying-jeanne ying-jeanne force-pushed the add-info-open-metrics branch from c415611 to d719da4 Compare June 14, 2024 17:07
@ying-jeanne ying-jeanne force-pushed the add-info-open-metrics branch from d719da4 to a93f747 Compare June 14, 2024 19:22
@ying-jeanne ying-jeanne force-pushed the add-info-open-metrics branch 2 times, most recently from b8fa11b to 7ba728d Compare July 18, 2024 23:35
@ying-jeanne ying-jeanne force-pushed the add-info-open-metrics branch from 7ba728d to 806e1d9 Compare July 18, 2024 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants