-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Update OM 2.0 with proposal#43 Created Timestamp syntax #2636
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
Conversation
ee584fd
to
f24c2ab
Compare
77a1036
to
8d8e083
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with some small corrections. Also as per WG decision, you'll need another approval.
Thanks for clarifying about exemplars :) |
I wonder if @ct needs to be prohibited explicitly for gauges, gauge histograms and alike. The ABNF does allow it, so syntax doesn't restrict, needs to be a semantic rule. |
Signed-off-by: Arthur Silva Sens <[email protected]>
Signed-off-by: Arthur Silva Sens <[email protected]>
Co-authored-by: George Krajcsovits <[email protected]> Signed-off-by: Arthur Silva Sens <[email protected]>
Co-authored-by: George Krajcsovits <[email protected]> Signed-off-by: Arthur Silva Sens <[email protected]>
Co-authored-by: George Krajcsovits <[email protected]> Signed-off-by: Arthur Silva Sens <[email protected]>
Signed-off-by: Arthur Silva Sens <[email protected]>
Signed-off-by: Arthur Silva Sens <[email protected]>
c5566b9
to
68c966d
Compare
Comments addressed, thanks for the review!
Do we mean that we need to be explicit in the Gauge/Gauge Histogram definition that they do not have created timestamps? The current definition doesn't say that created timestamps exist for them, so we can rely on the implicit information that those types don't have CT...? |
@krajorama, do you want to take another look or am I ok to merge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok to merge this with one comment about testing, see there.
I'd like to open a PR on top of this though to rename Created Value
to Created Timestamp
for clarity and make the doc consistent throughout. I feel that would read better, but is a separate question from changing _created to ct@.
Signed-off-by: George Krajcsovits <[email protected]>
Updates OpenMetrics 2.0 with our discussions in the working group and prometheus/proposals#43
cc @Maniktherana