-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[ resource generation processor] add new processor skeleton and configuration examples #3266
[ resource generation processor] add new processor skeleton and configuration examples #3266
Conversation
Related PR, which just wants to add the ability to scale metrics to the existing metricstransformprocessor: #3177 |
@bogdandrutu Would you please take a look on this? |
Hi @bogdandrutu a review is really appreciated. |
metricsgeneration: | ||
|
||
# specify the metric generation rules | ||
generation_rules: |
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.
Would simply rules
be a better name for this field? I imagine it's always going to follow the declaration of the metricsgeneration
processor and seems to stutter in that way. Similarly, some of the rule field names could be simplified by making use of their context:
processors:
metricsgeneration:
rules:
- name: <new metric name>
type: {calculate, scale}
metric1: <first metric>
metric2: <second metric>
operation: ...
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.
Makes sense. will update.
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.
updated.
cfg config.Processor, | ||
nextConsumer consumer.Metrics, | ||
) (component.MetricsProcessor, error) { | ||
processorConfig := cfg.(*Config) |
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.
processorConfig := cfg.(*Config) | |
processorConfig, ok := cfg.(*Config) | |
if !ok { | |
// handle this error | |
} |
This coercion should be checked to avoid a potential panic.
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.
Updated.
type internalGenerationRule struct { | ||
NewMetricName string | ||
Type GenerationType | ||
Operand1Metric string | ||
Operand2Metric string | ||
Operation OperationType | ||
ScaleBy float64 | ||
} |
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.
Why is there a new type that mirrors the exported GenerationRule
type?
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.
Updated.
} | ||
|
||
if !rule.Type.isValid() { | ||
return fmt.Errorf("%q must be in %q", GenerationTypeFieldName, generationTypes) |
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.
generationTypes
is now a map[GenerationType]struct{}
, so is this output still appropriate? Same with operationTypes
below.
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 don't think I understand what you said.
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.
https://play.golang.org/p/B6CLhHkdkDj
Map: map["bar":{} "foo":{}]
Slice: ["foo" "bar"]
MapKeys: ["foo" "bar"]
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.
Oh I see. Thanks. will update.
return fmt.Errorf("missing required field %q for generation type %q", Operand2MetricFieldName, Calculate) | ||
} | ||
|
||
if rule.Type == Scale && rule.ScaleBy <= 0 { |
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.
If addition is a valid operation should we allow 0
? I'm not sure whether it makes sense to use this to copy a metric.
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.
For generation type calculate
or scale
I think this seems to be OK. For renaming, we may introduce another generation type rename
which will filter based on metric labels and create a new one. The rename was part of our first proposal. But right now this is out of scope and we can achieve it with metricstransformprocessor
.
- new_metric_name: pod.cpu.utilized | ||
generation_type: calculate | ||
operand1_metric: pod.cpu.usage | ||
operand2_metric: node.cpu.limit | ||
operation: divide |
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 still think the rest of these field names should be simplified as well:
processors:
metricsgeneration:
rules:
- name: <new metric name>
type: {calculate, scale}
metric1: <first metric>
metric2: <second metric>
operation: ...
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 prefer generation_type
so that people don't mix it up with metric type. Also, I saw other examples where we used the prefix operand1_
which gives them clear idea about numerator and denominator. This design was discussed and I feel like current namings are OK. Expecting a second thought.
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.
v0v
I feel that the current names are overly verbose and somewhat awkward, but it's not a hill I want to fight on.
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.
Let's go with @Aneurysm9's suggested names, they look much better to me
pls rebase |
8bcc9eb
to
f805fef
Compare
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!
- new_metric_name: pod.cpu.utilized | ||
generation_type: calculate | ||
operand1_metric: pod.cpu.usage | ||
operand2_metric: node.cpu.limit | ||
operation: divide |
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.
Let's go with @Aneurysm9's suggested names, they look much better to me
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.
Just a couple more renames, thanks
Signed-off-by: Rayhan Hossain <[email protected]>
Co-authored-by: Anthony Mirabella <[email protected]>
Co-authored-by: Anthony Mirabella <[email protected]>
Co-authored-by: Anthony Mirabella <[email protected]>
Co-authored-by: Anthony Mirabella <[email protected]>
Co-authored-by: Anthony Mirabella <[email protected]>
Co-authored-by: Anthony Mirabella <[email protected]>
Co-authored-by: Anthony Mirabella <[email protected]>
Co-authored-by: Anthony Mirabella <[email protected]>
Co-authored-by: Anthony Mirabella <[email protected]>
Co-authored-by: Anthony Mirabella <[email protected]>
Co-authored-by: Anthony Mirabella <[email protected]>
Co-authored-by: Anthony Mirabella <[email protected]>
Signed-off-by: Rayhan Hossain <[email protected]>
Signed-off-by: Rayhan Hossain <[email protected]>
Signed-off-by: Rayhan Hossain <[email protected]>
Signed-off-by: Rayhan Hossain <[email protected]>
Signed-off-by: Rayhan Hossain <[email protected]>
Co-authored-by: Min Xia <[email protected]>
Signed-off-by: Rayhan Hossain <[email protected]>
Signed-off-by: Rayhan Hossain <[email protected]>
Signed-off-by: Rayhan Hossain <[email protected]>
Signed-off-by: Rayhan Hossain <[email protected]>
Signed-off-by: Rayhan Hossain <[email protected]>
Signed-off-by: Rayhan Hossain <[email protected]>
Co-authored-by: Punya Biswal <[email protected]>
0859749
to
aae5dc5
Compare
Rebased again to see if we can pass the unexpected windows and trace test failure. Now the load-test for logs is failing which is not related to this change. |
@bogdandrutu this PR has been reviewed by approved and should be ready-to-merge. |
Signed-off-by: Rayhan Hossain [email protected]
This PR adds the skeleton and configuration examples of a new processor
metricsgenerationprocessor
.Description:
The metrics generation processor (
metricsgenerationprocessor
) can be used to create new metrics using existing metrics following a given rule. Currently it supports following two approaches for creating a new metric.pod.memory.utilization
metric like the following equation-pod.memory.utilization
= (pod.memory.usage.bytes
/node.memory.limit
)pod.memory.usage
metric values from Megabytes to Bytes (multiply the existing metric's value by 1000000)Configuration
Configuration is specified through a list of generation rules. Generation rules find the metrics which
matche the given metric names and apply the operation to those metrics.
Example Configurations
Create a new metric using two existing metrics
Create a new metric scaling the value of an existing metric
Link to tracking Issue:
#2722
Testing:
Added unit tests. This is the skeleton only. Will add more test later.
Documentation:
README updated.