Add dimensions managedmetrics #106
Conversation
There was a problem hiding this comment.
Hi @jpraychev, thanks for your contribution 👍 I added a couple of comments. Since Metrics and FederatedMetrics both use projections, we should also update FederatedManagedMetrics if we add dimensions/projections to ManagedMetrics to keep things consistent. It feels a little bit like we reduce the value of a ManagedMetric by dropping the status conditions and letting the user defines this by themselves. When would I use a ManagedMetric over a Metric when they are essentially the same from a user perspective? Really interested in your thoughts regarding this.
|
Hello @christophrj and @jpraychev, I’m currently testing the Metrics Operator for use in a production environment. As I understand it, the key difference is that a I want to track all managed resources across the clusters, including their namespace, name, and status conditions. In the future, I may also need a specific label/annotation, plus the timestamp and message from the status condition. Allowing users to define their own projections (dimensions) in Yes I also think that dropping the status conditions by default would be confusing and would reduce the value of a |
You can specify GVK on both but as far as I know if GVK is specified for managed metrics it will drop everything else that is different than your specified GVK and drop all resources that are not tagged as crossplane/managed.
If you track all managed resources through single managed metric, all resources should share the same common fields, such as conditions, namespace, names, etc.
Sounds fair to toggle on/off status conditions as most of the time, people might want to include them in the metric itself without explicitly specifying them - maybe not all statuses but only ready/synced state. |
Yes that is the behaviour that i am seeing.
I think that is a good idea. |
|
instead of a user having to explicitely having to opt-in/out through a boolen flag, I would prefer it being implicitly handled depending on whether a user specifies dimension/projections or not.
|
|
Hi @christophrj Thinking about it, makes more sense. Let me work on that and update the PR. |
jpraychev
force-pushed
the
add-dimensions-managedmetrics
branch
from
2487993 to
222c8a4
Compare
|
Hey @christophrj Added the old behavior in case dimension field is not specified - if it is, user is responsible to add w/e dimensions they require. |
| for key, expr := range h.metric.Spec.Dimensions { | ||
| s, _, err := nestedPrimitiveValue(*u, expr) | ||
| if err != nil { | ||
| fmt.Printf("WARN: Could not parse expression '%s' for dimension field '%s'. Error: %v\n", key, expr, err) |
There was a problem hiding this comment.
please use the log.FromContext(ctx) logger Error() instead
| dataPoint.AddDimension(t, strconv.FormatBool(state)) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
nit: redundant empty line
3 participants
What this PR does / why we need it:
The PR adds a new field to managed metrics CR called dimensions, the purpose of which is to allow users to define their own dimensions. The PR also removes the old logic that extract
typeandstatusfrom conditions array, because now users can pick the needed conditions that would like to see as dimensions.Which issue(s) this PR fixes:
Fixes #49
Special notes for your reviewer:
No tests have been introduced with this PR as its purpose is to evaluate the dimension logic first.
Release note: