-
Notifications
You must be signed in to change notification settings - Fork 686
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 licence usage metrics #3145
Add licence usage metrics #3145
Conversation
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.
Overall looks good!
@@ -0,0 +1,58 @@ | |||
// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one |
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'm wondering if we should keep it close to the license stuff for the time being (pkg/license/metrics.go
) instead of using a generic metrics package. And maybe extract the generic metrics stuff once we need it somewhere else?
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 was wondering that too, but it also might be my personal twitch I get for anything in a utils
directory. But since there's license info in here I'm +1 for that I think
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 am planning on adding more metrics soon. Other metrics are a bit harder to integrate because the code is not organised with instrumentation in mind so I'll be doing that incrementally in a series of PRs.
I prefer the all-metrics-in-a-single-place approach as it is easier to audit and helps to implicitly enforce conventions on new metrics -- which is harder to do when they are strewn all over the place.
pkg/license/license.go
Outdated
@@ -57,16 +59,21 @@ func (r LicensingResolver) ToInfo(totalMemory resource.Quantity) (LicensingInfo, | |||
licenseLevel := r.getOperatorLicenseLevel(operatorLicense) |
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 would not expect ToInfo
to also update the in-memory metrics as a side-effect. Should we extract the metrics update to its own function (or rename this function)?
jenkins test this please |
@@ -62,6 +62,7 @@ func (r ResourceReporter) Report() error { | |||
return err | |||
} | |||
|
|||
licensingInfo.ReportAsMetrics() |
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.
The comment on this func could use an update since it's doing more now
assert.NoError(t, err) | ||
assert.Equal(t, "21.47GB", licensingInfo.TotalManagedMemory) | ||
assert.Equal(t, "1", licensingInfo.EnterpriseResourceUnits) | ||
func TestGet(t *testing.T) { |
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 realize both are not directly from this PR, but:
It doesn't look like we have a test exercising the maxERUs branch, correct? It might be worth adding one if possible
Nit: Since they don't share anything, at this point it might be simpler to break each case out into separate tests just to make it easier to run a specific test if you need to
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.
There are tests for maxERUs in the licence tests.
I am not sure I understand your comment about individual tests. You can already run specific tests by invoking something like go test -run='TestGet/elasticsearch_defaults' ./pkg/license/...
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.
You can already run specific tests
You're right, I was being goofy 🤦♀️
LGTM, nice work. Looks like most recent CI failure should be fixed with #3157 |
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
Jenkins test this please |
Exposes licence usage information as Prometheus metrics.
Metrics output:
ConfigMap output:
Fixes #3140