Add OpenMetrics unit support #544

vesari · 2023-12-02T15:37:50Z

Dependency of PR prometheus/client_golang#1392

vesari · 2023-12-07T12:31:50Z

@ArthurSens this is now ready for review, thanks.

ArthurSens · 2023-12-07T20:48:20Z

Implementation and tests looks good to me!

On the other hand, we had a brief discussion on slack and we suspect that enforcing units to be present in the MetricFamily name is a breaking change that we should be careful about.

We know that client_golang uses prometheus/common but maybe there are others? We could make this method opt-in with something like this

type ToOpenMetrics struct {
  withUnit bool
  // withCreated bool can be added in the future
}

type ToOpenMetricsOption = func(*ToOpenMetrics)

func ToOpenMetricsWithUnit() ToOpenMetricsOption {
  return func(om *ToOpenMetrics) {
    om.withUnit = true
  }
}

func MetricFamilyToOpenMetrics(out io.Writer, in *dto.MetricFamily, options ...ToOpenMetricsOption) (written int, err error) {
  toOM = ToOpenMetrics{}
  for _, option := range options {
    option(&toOM)
  }
.
.
.
  if toOM.withUnit && in.Unit != nil {
  // print Unit as usual
  }
.
.
.
}

Downstream projects could safely bump promethes/common version, while having the option to adopt units when appropriate:

// Will keep the same behavior as today, even if unit is set
common.MetricFamilyToOpenMetrics(out, in)

// Will add unit to the metricFamily name as suffix
common.MetricFamilyToOpenMetrics(out, in, common.ToOpenMetricsWithUnit())

vesari · 2023-12-15T12:47:24Z

@ArthurSens this is now ready for review. I hope I have interpreted correctly both your suggestions and the gist of your Slack conversation.

ArthurSens

Great improvement! Have you tried using this version of prometheus/common in your PR in client_golang? How did the changes here impacted over there?

I suspect we also need some changes to expfmt.NewEnconder as well.

ArthurSens · 2024-01-17T23:01:09Z

pinging @beorn7 and @bwplotka, maybe you also have some input here

ArthurSens

Could we also have some tests for the case where the unit is set, but the option withUnit is not?

beorn7 · 2024-01-18T12:46:37Z

Looks good in general. I don't have the time for a detailed review, but I trust you to do that alright on your own.

I should also add that I don't feel qualified to make any calls how to handle OpenMetrics and its quirks appropriately. (Not that I wouldn't have context. It's more that I have too much context, and that clouds my judgement.)

vesari · 2024-01-23T10:25:41Z

Great improvement! Have you tried using this version of prometheus/common in your PR in client_golang? How did the changes here impacted over there?

I suspect we also need some changes to expfmt.NewEnconder as well.

Thank you very much :) I have tried it on my branch (prometheus/client_golang#1392) and nothing seems to break or fail; however, I'll have to check again once I've modified expfmt.NewEncoder (thanks a lot for pointing that out!). I'll now convert this PR to draft for the time being, while I carry out the new changes and figure out their impact and ramifications in client_golang.

vesari · 2024-01-23T10:27:00Z

Could we also have some tests for the case where the unit is set, but the option withUnit is not?

That scenario is already covered by test n.10, but I now realise I could have named it more clearly, and of course I can add more tests.

ArthurSens

It looks almost ready! Thanks refactoring TestEncode with a table test, looks a lot better ❤️

I've made a few comments, but they are mostly around readability

ArthurSens · 2024-01-25T19:48:14Z

expfmt/openmetrics_create.go

+// function. It enables a set unit to be written to the output and to be added to the metric name, (if
+// it's not there already), as a suffix. Without opting in this way, the unit will not be added to the


Suggested change

// function. It enables a set unit to be written to the output and to be added to the metric name, (if

// it's not there already), as a suffix. Without opting in this way, the unit will not be added to the

// function. It enables a set unit to be written to the output and to be added to the metric name, if

// it's not there already, as a suffix. Without opting in this way, the unit will not be added to the

just small nit: no need for parenthesis if already separated from the main sentence with commas

ArthurSens · 2024-01-25T19:57:57Z

expfmt/encode_test.go

-	var buff bytes.Buffer
-	delimEncoder := NewEncoder(&buff, FmtProtoDelim)
-	metric := &dto.MetricFamily{
+	metric1 := &dto.MetricFamily{


What do you think about adding unit to metric1, so we don't need to create extra metrics for the OM format scenarios?

The OM Format is the only one that has a different output if unit is set, so I expect that this shouldn't require changes to the other formats

ArthurSens · 2024-01-25T19:58:31Z

go.mod

-	github.com/prometheus/client_model v0.5.0
+	github.com/prometheus/client_model v0.5.1-0.20231205151710-c26a8eea9e44


We probably want client_model 0.6.0 before merging this 🤔

ArthurSens · 2024-01-25T20:04:27Z

expfmt/encode_test.go

+		// N.B. For the first three cases, I'm maintaining the same checks as they were before, i. e. the expected output is
+		// not declared ergo no comparison with the actual output is ever performed, expect for its length being bigger than zero.
+		if i > 2 {
+			if expected, got := len(scenario.expOut), len(out.Bytes()); expected != got {
+				t.Errorf(
+					"%d. expected %d bytes written, got %d",
+					i, expected, got,
+				)
+			}
+			if expected, got := scenario.expOut, out.String(); expected != got {
+				t.Errorf(
+					"%d. expected out=%q, got %q",
+					i, expected, got,
+				)
+			}
+		}


Suggested change

// N.B. For the first three cases, I'm maintaining the same checks as they were before, i. e. the expected output is

// not declared ergo no comparison with the actual output is ever performed, expect for its length being bigger than zero.

if i > 2 {

if expected, got := len(scenario.expOut), len(out.Bytes()); expected != got {

t.Errorf(

"%d. expected %d bytes written, got %d",

i, expected, got,

)

}

if expected, got := scenario.expOut, out.String(); expected != got {

t.Errorf(

"%d. expected out=%q, got %q",

i, expected, got,

)

}

}

if expected, got := len(scenario.expOut), len(out.Bytes()); expected != "" && expected != got {

t.Errorf(

"%d. expected %d bytes written, got %d",

i, expected, got,

)

}

if expected, got := scenario.expOut, out.String(); expected != "" && expected != got {

t.Errorf(

"%d. expected out=%q, got %q",

i, expected, got,

)

}

We can remove one indentation level if we compare when the expected output is not empty. Is this more readable/maintainable to you?

ArthurSens · 2024-01-25T20:07:45Z

expfmt/encode_test.go

+		{
+			metric: metric1,
+			format: FmtProtoDelim,
+			// expOut: `\x1b\n\nfoo_metric\x18\x03\"\v*\t\tX9\xb4\xc8v\xbe\xf3?`, // does this look like the desired output?


The problem with comparing protos in string format is that the output is not consistent, as far as I know. Proto will randomly add spaces between messages, making our comparison flaky.

I'd remove this commented line entirely. We weren't comparing the output before, so I don't think we need to compare now :)

vesari · 2024-02-13T11:44:18Z

@ArthurSens thank you very much for your guidance! I've addressed your change requests. This is now ready for review. About the client_model version upgrade, I’m not sure how to proceed, as I would think only maintainers are allowed to change it? Thanks for letting me know :)

vesari · 2024-02-29T12:48:21Z

@ArthurSens @bwplotka , could you please take a look? Since v0.6.0. of client_model has been adopted yesterday by this repo, unless you see something not correct in this PR, I guess it could be merged? Thanks a lot for your time as per usual :)

SuperQ · 2024-02-29T15:09:37Z

@vesari would you mind rebasing and squashing the commits?

vesari · 2024-02-29T16:13:30Z

@SuperQ done. Thank you!

ArthurSens

@vesari LGTM! thanks for your patience through all those months :)

I've noticed that a few unrelated changes were made, probably by mistake? Could you take a look?

ArthurSens · 2024-03-02T19:11:08Z

model/labelset.go

-// String will look like `{foo="bar", more="less"}`. Names are sorted alphabetically.
 func (l LabelSet) String() string {
-	var lna [32]LabelName // On stack to avoid memory allocation for sorting names.
-	labelNames := lna[:0]
-	for name := range l {
-		labelNames = append(labelNames, name)
+	lstrs := make([]string, 0, len(l))
+	for l, v := range l {
+		lstrs = append(lstrs, fmt.Sprintf("%s=%q", l, v))
 	}
-	slices.Sort(labelNames)
-	var bytea [1024]byte // On stack to avoid memory allocation while building the output.
-	b := bytes.NewBuffer(bytea[:0])
-	b.WriteByte('{')
-	for i, name := range labelNames {
-		if i > 0 {
-			b.WriteString(", ")
-		}
-		b.WriteString(string(name))
-		b.WriteByte('=')
-		b.Write(strconv.AppendQuote(b.AvailableBuffer(), string(l[name])))
-	}
-	b.WriteByte('}')
-	return b.String()
+
+	sort.Strings(lstrs)
+	return fmt.Sprintf("{%s}", strings.Join(lstrs, ", "))


Accidental change with the rebase?

ArthurSens · 2024-03-02T19:11:29Z

model/labelset_test.go

-
-// Benchmark Results for LabelSet's String() method
-// ---------------------------------------------------------------------------------------------------------
-// goos: linux
-// goarch: amd64
-// pkg: github.com/prometheus/common/model
-// cpu: 11th Gen Intel(R) Core(TM) i5-1145G7 @ 2.60GHz
-// BenchmarkLabelSetStringMethod-8                               732376              1532 ns/op
-
-func BenchmarkLabelSetStringMethod(b *testing.B) {
-	ls := make(LabelSet)
-	ls["monitor"] = "codelab"
-	ls["foo2"] = "bar"
-	ls["foo"] = "bar"
-	ls["abc"] = "prometheus"
-	ls["foo11"] = "bar11"
-	for i := 0; i < b.N; i++ {
-		_ = ls.String()
-	}
-}


same thing here, probably by accident?

vesari · 2024-03-02T19:56:59Z

@ArthurSens thanks a lot for pointing that out, I'll have a look at it tomorrow morning then get back to you immediately after, thanks again

Signed-off-by: Arianna Vespri <[email protected]>

vesari · 2024-03-03T10:15:42Z

@ArthurSens , I have now amended my previous commit and rebased again. Thank you very much for catching those 2 files that I had indeed not modified actively; like you, I can just assume something happened during the previous rebase. I hope now it's ready for merging :)

ArthurSens · 2024-03-04T14:23:26Z

Awesome, could someone with merge rights also give a review?

Maybe @bwplotka, @SuperQ or @beorn7 ?

SuperQ

Thanks!

vesari force-pushed the add-unit branch from 56657f4 to 53a2259 Compare December 2, 2023 15:43

vesari marked this pull request as ready for review December 7, 2023 12:29

ArthurSens reviewed Jan 17, 2024

View reviewed changes

vesari marked this pull request as draft January 23, 2024 10:27

ArthurSens reviewed Jan 25, 2024

View reviewed changes

vesari marked this pull request as ready for review February 13, 2024 11:27

SuperQ requested a review from ArthurSens February 29, 2024 15:08

vesari force-pushed the add-unit branch from 5bcff99 to 112a82d Compare February 29, 2024 16:10

ArthurSens approved these changes Mar 2, 2024

View reviewed changes

Add unit

6a86956

Signed-off-by: Arianna Vespri <[email protected]>

vesari force-pushed the add-unit branch from 112a82d to 6a86956 Compare March 3, 2024 10:09

SuperQ requested a review from beorn7 March 6, 2024 13:20

SuperQ approved these changes Mar 6, 2024

View reviewed changes

SuperQ merged commit 1e6ac24 into prometheus:main Mar 6, 2024
6 checks passed

bboreham changed the title ~~Add unit~~ Add OpenMetrics unit support Mar 6, 2024

bboreham mentioned this pull request Mar 7, 2024

Update Go dependencies for v2.51 prometheus/prometheus#13716

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OpenMetrics unit support #544

Add OpenMetrics unit support #544

vesari commented Dec 2, 2023 •

edited

Loading

vesari commented Dec 7, 2023

ArthurSens commented Dec 7, 2023 •

edited

Loading

vesari commented Dec 15, 2023

ArthurSens left a comment

ArthurSens commented Jan 17, 2024

ArthurSens left a comment

beorn7 commented Jan 18, 2024

vesari commented Jan 23, 2024

vesari commented Jan 23, 2024

ArthurSens left a comment

ArthurSens Jan 25, 2024

ArthurSens Jan 25, 2024

ArthurSens Jan 25, 2024

ArthurSens Jan 25, 2024

ArthurSens Jan 25, 2024

vesari commented Feb 13, 2024

vesari commented Feb 29, 2024

SuperQ commented Feb 29, 2024

vesari commented Feb 29, 2024

ArthurSens left a comment

ArthurSens Mar 2, 2024

ArthurSens Mar 2, 2024

vesari commented Mar 2, 2024

vesari commented Mar 3, 2024

ArthurSens commented Mar 4, 2024

SuperQ left a comment

		// function. It enables a set unit to be written to the output and to be added to the metric name, (if
		// it's not there already), as a suffix. Without opting in this way, the unit will not be added to the

		github.com/prometheus/client_model v0.5.0
		github.com/prometheus/client_model v0.5.1-0.20231205151710-c26a8eea9e44

Add OpenMetrics unit support #544

Add OpenMetrics unit support #544

Conversation

vesari commented Dec 2, 2023 • edited Loading

vesari commented Dec 7, 2023

ArthurSens commented Dec 7, 2023 • edited Loading

vesari commented Dec 15, 2023

ArthurSens left a comment

Choose a reason for hiding this comment

ArthurSens commented Jan 17, 2024

ArthurSens left a comment

Choose a reason for hiding this comment

beorn7 commented Jan 18, 2024

vesari commented Jan 23, 2024

vesari commented Jan 23, 2024

ArthurSens left a comment

Choose a reason for hiding this comment

ArthurSens Jan 25, 2024

Choose a reason for hiding this comment

ArthurSens Jan 25, 2024

Choose a reason for hiding this comment

ArthurSens Jan 25, 2024

Choose a reason for hiding this comment

ArthurSens Jan 25, 2024

Choose a reason for hiding this comment

ArthurSens Jan 25, 2024

Choose a reason for hiding this comment

vesari commented Feb 13, 2024

vesari commented Feb 29, 2024

SuperQ commented Feb 29, 2024

vesari commented Feb 29, 2024

ArthurSens left a comment

Choose a reason for hiding this comment

ArthurSens Mar 2, 2024

Choose a reason for hiding this comment

ArthurSens Mar 2, 2024

Choose a reason for hiding this comment

vesari commented Mar 2, 2024

vesari commented Mar 3, 2024

ArthurSens commented Mar 4, 2024

SuperQ left a comment

Choose a reason for hiding this comment

vesari commented Dec 2, 2023 •

edited

Loading

ArthurSens commented Dec 7, 2023 •

edited

Loading