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

Optionally print OM created lines #1408

Merged
merged 1 commit into from
Oct 14, 2024
Merged

Optionally print OM created lines #1408

merged 1 commit into from
Oct 14, 2024

Conversation

ArthurSens
Copy link
Member

@ArthurSens ArthurSens commented Dec 12, 2023

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.

@ArthurSens ArthurSens changed the title Optionally print OM created lines [WIP] Optionally print OM created lines Dec 12, 2023
@ArthurSens ArthurSens changed the title [WIP] Optionally print OM created lines Optionally print OM created lines Feb 2, 2024
@ArthurSens ArthurSens force-pushed the om-created-lines branch 3 times, most recently from e930252 to 240345e Compare February 2, 2024 18:35
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.

Thanks! One minor nit, otherwise well done!

prometheus/promhttp/http.go Outdated Show resolved Hide resolved
@ArthurSens ArthurSens force-pushed the om-created-lines branch 2 times, most recently from 33ed3e6 to e9c6f77 Compare February 18, 2024 18:33
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.

Epic, proposed some improvements, thanks!

prometheus/promhttp/http.go Outdated Show resolved Hide resolved
prometheus/promhttp/http.go Outdated Show resolved Hide resolved
prometheus/promhttp/http.go Show resolved Hide resolved
prometheus/promhttp/http.go Outdated Show resolved Hide resolved
prometheus/promhttp/http.go Outdated Show resolved Hide resolved
prometheus/promhttp/http.go Outdated Show resolved Hide resolved
prometheus/promhttp/http.go Outdated Show resolved Hide resolved
@ArthurSens
Copy link
Member Author

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

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.

LGTM

@ArthurSens
Copy link
Member Author

After quite some work to unblock this, this is finally ready :)

Do you want to review again or should I just merge?

@ArthurSens
Copy link
Member Author

Now that prometheus/prometheus#14356 was merged, should we revive this? :)

@ArthurSens
Copy link
Member Author

PR rebased on main. Ready for another review just in case @kakkoyun @bwplotka @vesari

@ArthurSens
Copy link
Member Author

Since we have approvals from the past, I'll take the liberty to merge this PR next week :)

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.

Let's timebox this, but wanted to try last more time to consider an improvement to the config structure, otherwise LGTM!

prometheus/promhttp/http.go Outdated Show resolved Hide resolved
Copy link

@Maniktherana Maniktherana left a 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?

@ArthurSens
Copy link
Member Author

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 🤷

@ArthurSens ArthurSens force-pushed the om-created-lines branch 2 times, most recently from 3edabb8 to 1523562 Compare October 8, 2024 12:52
@ArthurSens
Copy link
Member Author

@bwplotka should be ready for review again!

Signed-off-by: Arthur Silva Sens <arthur.sens@coralogix.com>
Copy link
Member

@krajorama krajorama left a comment

Choose a reason for hiding this comment

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

SGTM

@ArthurSens
Copy link
Member Author

@bwplotka do you want to take one final look before I merge :)

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.

LGTM, thanks!

@bwplotka bwplotka merged commit 284ca0f into main Oct 14, 2024
12 checks passed
@bwplotka bwplotka deleted the om-created-lines branch October 14, 2024 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OpenMetrics OpenMetrics v1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants