-
Notifications
You must be signed in to change notification settings - Fork 241
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
Enable the resource_to_telemetry_conversion to true in amp config.yaml files #739
Enable the resource_to_telemetry_conversion to true in amp config.yaml files #739
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.
Oh it looks like this setting is available, cool.
It's not documented at all 😱
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.
Unfortunately, unlike the EMF exporter setting, this does not seem to support a list of labels to allow list
We can't enable this because extremely high cardinality resource information will be sent and kill the prometheus. Perhaps that's why it's not documented :P
Codecov Report
@@ Coverage Diff @@
## main #739 +/- ##
=======================================
Coverage 54.69% 54.69%
=======================================
Files 11 11
Lines 298 298
=======================================
Hits 163 163
Misses 118 118
Partials 17 17 Continue to review full report at Codecov.
|
Thanks for the comments, will determine the need with teams. Please find the related PR and issues |
@anuraaga Agree with your concerns. Each service has to take a call on cardinality related performance concerns. |
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.
This is approved for AMP based on feedback from service team on no known limitations.
I'm still worried about sending resource data without an allow list - assuming the cardinality can be handled without slowing down queries, it's still a lot of storage used (process.command_line can be on the kilobyte scale for some apps) and could cause cost balooning without value to customers. |
@anuraaga The service team has confirmed that they can handle high cardinality data. |
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 based on service team feedback.
This PR will add dimensions/tags and more detailed information to AMP metrics which helps to identify the metrics using cluster_name, task_id etc.,
Modified the following ecs-amp config files by enabling the resource_to_telemetry_conversion to true
Paths to config files
config/ecs/ecs-amp.yaml
config/ecs/ecs-amp-xray.yaml
config/ecs/ecs-amp-xray-prometheus.yaml
config/ecs/ecs-amp-prometheus.yaml