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 openmetric parser[1]: add type,unit,help parser #710

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jyz0309
Copy link

@jyz0309 jyz0309 commented Oct 28, 2024

Ref: prometheus/prometheus#8932
Part of #669
Add openmetrics protocol decoder, for promtool to support check openmetric protocol.
This pr just parse the HELP, TYPE, UNIT as the comment suggest

Copy link
Member

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

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

Good work here! I can see that this is basically a copy of the current text parser alongside its tests, but I think we can improve things a bit.

Comments are mostly around test readability

expfmt/openmetrics_parse_test.go Outdated Show resolved Hide resolved
expfmt/openmetrics_parse_test.go Outdated Show resolved Hide resolved
expfmt/openmetrics_parse_test.go Outdated Show resolved Hide resolved
Comment on lines +217 to +225
scenarios := []struct {
in string
err string
}{
// 0:
{
in: `# TYPE metric counter
# TYPE metric untyped
`,
Copy link
Member

Choose a reason for hiding this comment

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

Let's do the same here! Adding name to the scenarios and then using t.Run should provide more context about what each scenario is about :)

Copy link
Author

Choose a reason for hiding this comment

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

has a little confusing about this, because now there's benchmark for the test, so the arg in test function is testing.TB, and arg required by t.Run is *testing.T(t.Run(name string, f func(t *testing.T))), the testing.TB has not implement the interface

expfmt/openmetrics_parse.go Show resolved Hide resolved
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.

2 participants