-
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 add OM unit #1392
Optionally add OM unit #1392
Changes from all commits
d74a7fb
5d8c6de
f78b666
e876012
b1f5a86
788d53b
1692495
5971215
efb64f3
fafe45b
51861d5
1362436
a2bfd3a
7900714
19d7102
031e0d3
fd60a57
f7e2848
5f58996
ac9d1d8
3db440e
7752d37
47c8cf1
7312bd5
3644321
dca67a5
ab34546
717d3d3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,6 +47,8 @@ type Desc struct { | |
fqName string | ||
// help provides some helpful information about this metric. | ||
help string | ||
// unit provides the unit of this metric. | ||
unit string | ||
// constLabelPairs contains precalculated DTO label pairs based on | ||
// the constant labels. | ||
constLabelPairs []*dto.LabelPair | ||
|
@@ -75,8 +77,8 @@ type Desc struct { | |
// | ||
// For constLabels, the label values are constant. Therefore, they are fully | ||
// specified in the Desc. See the Collector example for a usage pattern. | ||
func NewDesc(fqName, help string, variableLabels []string, constLabels Labels) *Desc { | ||
return V2.NewDesc(fqName, help, UnconstrainedLabels(variableLabels), constLabels) | ||
func NewDesc(fqName, help, unit string, variableLabels []string, constLabels Labels) *Desc { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately this is a breaking change. We can't change this function. Not sure how to solve it. Probably NewDescWithUnit, or changing v2 only would make sense. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, we can't make this change. I wonder what would be a good option though, maybe something like below? func (d *Desc) WithUnit(u string) *Desc {
d.unit = u
return d
} A new function There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, I'll revert the breaking changes and explore the alternatives you both suggested. |
||
return V2.NewDesc(fqName, help, unit, UnconstrainedLabels(variableLabels), constLabels) | ||
} | ||
|
||
// NewDesc allocates and initializes a new Desc. Errors are recorded in the Desc | ||
|
@@ -89,10 +91,11 @@ func NewDesc(fqName, help string, variableLabels []string, constLabels Labels) * | |
// | ||
// For constLabels, the label values are constant. Therefore, they are fully | ||
// specified in the Desc. See the Collector example for a usage pattern. | ||
func (v2) NewDesc(fqName, help string, variableLabels ConstrainableLabels, constLabels Labels) *Desc { | ||
func (v2) NewDesc(fqName, help, unit string, variableLabels ConstrainableLabels, constLabels Labels) *Desc { | ||
d := &Desc{ | ||
fqName: fqName, | ||
help: help, | ||
unit: unit, | ||
variableLabels: variableLabels.compile(), | ||
} | ||
if !model.IsValidMetricName(model.LabelValue(fqName)) { | ||
|
@@ -149,10 +152,11 @@ func (v2) NewDesc(fqName, help string, variableLabels ConstrainableLabels, const | |
d.id = xxh.Sum64() | ||
// Sort labelNames so that order doesn't matter for the hash. | ||
sort.Strings(labelNames) | ||
// Now hash together (in this order) the help string and the sorted | ||
// Now hash together (in this order) the help string, the unit string and the sorted | ||
// label names. | ||
xxh.Reset() | ||
xxh.WriteString(help) | ||
xxh.WriteString(unit) | ||
xxh.Write(separatorByteSlice) | ||
for _, labelName := range labelNames { | ||
xxh.WriteString(labelName) | ||
|
@@ -198,9 +202,10 @@ func (d *Desc) String() string { | |
} | ||
} | ||
return fmt.Sprintf( | ||
"Desc{fqName: %q, help: %q, constLabels: {%s}, variableLabels: {%s}}", | ||
"Desc{fqName: %q, help: %q, unit: %q, constLabels: {%s}, variableLabels: {%s}}", | ||
d.fqName, | ||
d.help, | ||
d.unit, | ||
strings.Join(lpStrings, ","), | ||
strings.Join(vlStrings, ","), | ||
) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,8 +52,8 @@ type InfoVec struct { | |
*prometheus.MetricVec | ||
} | ||
|
||
func NewInfoVec(name, help string, labelNames []string) *InfoVec { | ||
desc := prometheus.NewDesc(name, help, labelNames, nil) | ||
func NewInfoVec(name, help, unit string, labelNames []string) *InfoVec { | ||
desc := prometheus.NewDesc(name, help, unit, labelNames, nil) | ||
Comment on lines
-55
to
+56
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar problem here, we can't change Public function's signatures |
||
return &InfoVec{ | ||
MetricVec: prometheus.NewMetricVec(desc, func(lvs ...string) prometheus.Metric { | ||
if len(lvs) != len(labelNames) { | ||
|
@@ -110,6 +110,7 @@ func ExampleMetricVec() { | |
infoVec := NewInfoVec( | ||
"library_version_info", | ||
"Versions of the libraries used in this binary.", | ||
"", | ||
[]string{"library", "version"}, | ||
) | ||
|
||
|
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.
Next time, this probably should be done in a separate PR 🤗
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.
I wonder why this change was even needed 😬, could you explain why you decided to bump the go version here?
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.
Same as with the
go.mod
below. Just some change I should have reverted, sorry.