-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[metricstransformprocessor] add new operation- convert_resource_attributes_to_metric_labels #1337
[metricstransformprocessor] add new operation- convert_resource_attributes_to_metric_labels #1337
Conversation
…butes_to_metric_labels Signed-off-by: Rayhan Hossain <[email protected]>
|
||
## Configuration | ||
```yaml | ||
# transforms is a list of transformations with each element transforming a metric selected by metric name | ||
transforms: | ||
# name is used to match with the metric to operate on. This implementation doesn’t utilize the filtermetric’s MatchProperties struct because it doesn’t match well with what I need at this phase. All is needed for this processor at this stage is a single name string that can be used to match with selected metrics. The list of metric names and the match type in the filtermetric’s MatchProperties struct are unnecessary. Also, based on the issue about improving filtering configuration, it seems like this struct is subject to be slightly modified. | ||
|
||
# new addition: now it accepts `all_metrics` as the value of 'metric_name' field. This batch operation applies changes to all the metrics. However, currently, it only works and is tested for the 'convert_resource_attributes_to_labels' operation. For other operations (e.g. update_label, add_label), it may not work properly. But, it shouldn't break existing user experience. |
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.
Instead of adding this in the README, capture it in the PR description and file an issue to fix for other operations in the future.
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.
Created an issue: #1342
@@ -55,6 +58,9 @@ transforms: | |||
aggregated_values: [values...] | |||
new_value: <new_value> | |||
aggregation_type: {sum, mean, max} | |||
|
|||
# convert_resource_attributes_to_labels action converts all the resourde attributes to metric labels by default. Right now, it doesn't support selecting specific resource attributes as metric labels. |
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.
Ditto file an issue or update existing one to track the addition.
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.
Created an issue: #1343
@@ -570,6 +570,68 @@ var ( | |||
build(), | |||
}, | |||
}, | |||
{ |
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.
Is there a test case for the nil case?
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.
Added a new test case for nil or empty resource attributes.
|
||
## Configuration | ||
```yaml | ||
# transforms is a list of transformations with each element transforming a metric selected by metric name | ||
transforms: | ||
# name is used to match with the metric to operate on. This implementation doesn’t utilize the filtermetric’s MatchProperties struct because it doesn’t match well with what I need at this phase. All is needed for this processor at this stage is a single name string that can be used to match with selected metrics. The list of metric names and the match type in the filtermetric’s MatchProperties struct are unnecessary. Also, based on the issue about improving filtering configuration, it seems like this struct is subject to be slightly modified. | ||
|
||
# it also accepts `all_metrics` as the value of 'metric_name' field. This batch operation applies changes to all the metrics. However, currently, it only works and is tested for the 'convert_resource_attributes_to_labels' 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.
nit: Can you just implement it for all operations in this PR? Or create a separate PR for just the all_metrics change
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 most of the update operations, it should work fine. I already did some manual testing. Need more unit tests to verify it works fine for the operations we are claiming. Separate PR would make the process faster. Here is the issue I created for tracking- #1342 . Also, for the aggregation operations, I don't know how it will work. That's why planned for 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.
Thanks, nice feature
@tigrannajaryan Can you review this PR? It seems nice to me to be able to have one processor to do resource / label handling since it's such a common use case but let us know your thoughts :) |
Signed-off-by: Rayhan Hossain <[email protected]>
Sorry @anuraaga for pushing code after your approval. I just added the options for selecting specific resource attributes as metric labels. I feel it's a good option to have from the beginning and pretty simple to send with this PR. Would mind taking a look. |
// for other operations. After updating the metrics, it will continue for the next iteration. | ||
// TODO: add more unit tests and update README to pubicly anounce this feature for other | ||
// operations as well. | ||
if transform.MetricName == "all_metrics" { |
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.
Rather than use "all_metrics" as a special value, I think it would make more sense to do what @tigrannajaryan suggested and make MetricName
optional in the config. If left out, then the transform applies to all metrics.
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.
Pushed an update. Nice suggestion. Thanks @james-bebbington.
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.
@james-bebbington's suggestion sounds nice, otherwise the new allow-list code LGTM
…ration for all metrics Signed-off-by: Rayhan Hossain <[email protected]>
@tigrannajaryan can we get this merged. Thanks. |
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.
Requesting changes since I don't think this is the right approach. Please sync with @amanbrar1999 and @anuraaga
|
||
## Configuration | ||
```yaml | ||
# transforms is a list of transformations with each element transforming a metric selected by metric name | ||
transforms: | ||
# name is used to match with the metric to operate on. This implementation doesn’t utilize the filtermetric’s MatchProperties struct because it doesn’t match well with what I need at this phase. All is needed for this processor at this stage is a single name string that can be used to match with selected metrics. The list of metric names and the match type in the filtermetric’s MatchProperties struct are unnecessary. Also, based on the issue about improving filtering configuration, it seems like this struct is subject to be slightly modified. | ||
|
||
# if the value for `metric_name` is not set (empty string), the operations will be applied to all the metrics. However, currently, it only works and is tested for the 'convert_resource_attributes_to_labels' operation. Existing experience works fine with correct `metric_name`. |
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.
it only works and is tested for the 'convert_resource_attributes_to_labels' operation.
This is not what I was expecting. I don't think we want a modification like this.
We discussed this in the SIG meeting yesterday where @amanbrar1999 and @anuraaga were present. My opinion is that it is the responsibility of exporters to do this transformation. If we want a processor to do this then I would expect it to be a separate specialized processor, provided that for whatever reason we are unable to fix the exporter. I don't see this fitting well in the metricstransformprocessor. The new functionality is not a metric transformation. It is a transformation of a resource into metric labels.
However, ultimately as I said I think this is a bug/deficiency of the Prometheus exporter.
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.
it only works and is tested for the 'convert_resource_attributes_to_labels' operation.
**This is not what I was expecting. I don't think we want a modification like this.**
As I mentioned in the PR description, we created an issue [#1342] to work on that and it does not break existing user experience. I can work on that after this change or maybe with this change if community doesn't agree.
However, I don't clearly get why it should not go with metrictransform
processor. In a sense, it's adding new labels to the metrics which means transforming the existing metrics.
Also, I will sync up with @anuraaga and @amanbrar1999.
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.
To clarify myself- metricstransform
processor already has an operation add_label
and this incoming change is a similar operation which picks the labels from resource attributes. It's a simple new operation type which gives me a feeling to make it a part of this processor.
Also, our main target is awsemf
exporter not the prometheus
exporter. This tells us a strong point to keep this in the processor. What if our customer uses a new exporter where we don't have this feature. Then, we are kind of blocking our customers. Having this option in the processor will give our customer to choose any exporters they prefer.
Please correct me if I am wrong. Expecting a second thought @tigrannajaryan .
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.
However, I don't clearly get why it should not go with metrictransform processor. In a sense, it's adding new labels to the metrics which means transforming the existing metrics.
This proposal adds something that does not fit nicely the metrictransform's intent of transforming metrics. This proposal converts resources into metric labels, which is not a metric transformation.
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
…plicate code (#1337) Signed-off-by: Bogdan Drutu <[email protected]>
* Rename sdk/trace/attributesMap.go -> sdk/trace/attributesmap.go Signed-off-by: Daniil Rutskiy <[email protected]> * Add missing tests for attributesMap Signed-off-by: Daniil Rutskiy <[email protected]> * Update CHANGELOG.md Signed-off-by: Daniil Rutskiy <[email protected]> * Add missing license header Signed-off-by: Daniil Rutskiy <[email protected]> * Delete underscores in test names Signed-off-by: Daniil Rutskiy <[email protected]> * Tests clean up Signed-off-by: Daniil Rutskiy <[email protected]>
Description:
This change will
convert_resource_attributes_to_metric_labels
. By default, it converts all the resource attributes to metric labels. Customer can select specific resource attributes to set them as metric labels.convert_resource_attributes_to_metric_labels
. If customer does not set the value formetric_name
(empty string), it means the changes will be applied to all metrics. However, currently, it only works and is tested for theconvert_resource_attributes_to_labels
operation. For other operations (e.g.update_label
,add_label
), it may not work properly. But, it shouldn't break existing user experience. Open issue to test and verify this change for other operations [metricstransformprocessor] Enable batch operation (all_metrics) for processor operations #1342Link to tracking Issue:
#1297
#1343
Testing:
Documentation:
README updated