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

add _created support: add createdTime() method to counter #1044

Closed
wants to merge 2 commits into from

Conversation

andysim3d
Copy link

first step of issue #685

Signed-off-by: Pengfei Zhang zhangpengfei@google.com

Signed-off-by: Pengfei Zhang <zhangpengfei@google.com>
@andysim3d
Copy link
Author

@beorn7 Please take a look since you are the original owner of that issue.

@kakkoyun kakkoyun requested a review from beorn7 May 4, 2022 08:36
@beorn7
Copy link
Member

beorn7 commented May 4, 2022

While I opened this issue, this has to be figured out between the OpenMetrics experts and the current maintainers of prometheus/client_golang.

From a very general perspective, I don't think Created() should be a method (similar to not making the counter value itself accessible via some kind of Get() method).

@beorn7 beorn7 requested review from bwplotka and kakkoyun and removed request for beorn7 May 4, 2022 10:14
prometheus/counter.go Outdated Show resolved Hide resolved
Signed-off-by: Pengfei Zhang <zhangpengfei@google.com>
@andysim3d andysim3d requested a review from beorn7 May 5, 2022 16:21
@kakkoyun kakkoyun added this to the OpenMetrics support in v1 milestone Jun 17, 2022
Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

We haven't discussed OpenMetrics support among maintainers yet. cc @bwplotka
I'll add this to the milestone. And ask for your further patience for the decision.

@pintohutch
Copy link

Hello,

Is this still being discussed?

@logicalhan
Copy link
Contributor

Can we please do this? _created timeseries is a necessary prerequisite for upstream Kubernetes to even consider supporting prometheus' open-metrics format.

@kakkoyun
Copy link
Member

kakkoyun commented Mar 1, 2023

Can we please do this? _created timeseries is a necessary prerequisite for upstream Kubernetes to even consider supporting prometheus' open-metrics format.

I will check-in with maintainers about this again.

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Hi,

Thanks for this. We decided on Prometheus DevSummit to pursue proto version of OpenMetrics format as the main one. So I would implement that first.

For OM v1.0 text format, there are many weird conflicts and incompatibilities (e.g spam of extra metric on all Prometheus versions), so we can't really support it in client_golang and definitely not by default. I would suggest waiting on text improvement for OM e.g in OM 2.0 and rely on proto only.

WDYT?

valInt uint64
valBits uint64
valInt uint64
createdTime uint64
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this field is ever used, no?

Choose a reason for hiding this comment

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

It's initialized on L166 in initCreated() call.

Presumably, a forthcoming change would use it for serving in a Prometheus counter metric.

Copy link
Member

Choose a reason for hiding this comment

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

Yup, but we would need this to be either in this PR ideally so we see how end solution would look like 🤗

Definitely it's good direction!

@bwplotka
Copy link
Member

bwplotka commented Aug 2, 2023

BTW, @ArthurSens, our mentee works on adding created timestamp with tests #1044

@bwplotka
Copy link
Member

Done in #1044, so closing this PR. Thanks anyway!

@bwplotka bwplotka closed this Oct 10, 2023
@logicalhan
Copy link
Contributor

logicalhan commented Oct 10, 2023

Done in #1044, so closing this PR. Thanks anyway!

this is PR #1044...

@ArthurSens
Copy link
Member

I believe he meant #1313

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.

None yet

8 participants