-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Optionally print OM created lines #1408
Conversation
c26d312
to
b7d00b2
Compare
e930252
to
240345e
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.
Thanks! One minor nit, otherwise well done!
33ed3e6
to
e9c6f77
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.
Epic, proposed some improvements, thanks!
e9c6f77
to
81909c5
Compare
We need prometheus/common v0.49.0 for this feature, but this version of prometheus/common requires go 1.21. Therefore, the PR is blocked until we drop support for go 1.20 |
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.
LGTM
81909c5
to
65df2d7
Compare
After quite some work to unblock this, this is finally ready :) Do you want to review again or should I just merge? |
Now that prometheus/prometheus#14356 was merged, should we revive this? :) |
eb0f4ef
to
b51b530
Compare
Since we have approvals from the past, I'll take the liberty to merge this PR next week :) |
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.
Let's timebox this, but wanted to try last more time to consider an improvement to the config structure, otherwise LGTM!
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.
should we add summaries and counters in the example too?
So far all our examples just add one or two metrics, not sure if that's needed 🤷 |
3edabb8
to
1523562
Compare
@bwplotka should be ready for review again! |
Signed-off-by: Arthur Silva Sens <arthur.sens@coralogix.com>
1523562
to
00fb24c
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.
SGTM
@bwplotka do you want to take one final look before I 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.
LGTM, thanks!
Signed-off-by: Arthur Silva Sens <arthur.sens@coralogix.com> Co-authored-by: Arthur Silva Sens <arthur.sens@coralogix.com> Signed-off-by: Eugene <eugene@amberpixels.io>
This PR adds an extra option to print _created lines when OpenMetrics is the chosen exposition format. It's implemented following the conditional option added in prometheus/common#504.