Skip to content
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

Update ECS App Metrics default config for CloudWatch #749

Merged
merged 3 commits into from
Nov 17, 2021

Conversation

hossain-rayhan
Copy link
Contributor

@hossain-rayhan hossain-rayhan commented Nov 16, 2021

Signed-off-by: Rayhan Hossain hossain.rayhan@outlook.com

Description:
This PR will update our default ECS app metrics config for CloudWatch. We will offer a minimal set of metric dimensions/labels which are truly useful for our customers. For customers who want more dimensions (less common use case) can always come up with their custom config.

Proposed dimensions/labels:

  • ClusterARN (We can add ClusterName once it’s available from ADOT)
  • TaskARN
  • TaskDefinitionFamily
  • TaskDefinitionRevision
  • LaunchType
  • InstanceId (Only for EC2)
  • CustomerAddedDimensions (from Application or Environment Variables)

Signed-off-by: Rayhan Hossain <hossain.rayhan@outlook.com>
@codecov-commenter
Copy link

Codecov Report

Merging #749 (24a629c) into main (6f6e11e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #749   +/-   ##
=======================================
  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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f6e11e...24a629c. Read the comment docs.

@mxiamxia
Copy link
Member

mxiamxia commented Nov 16, 2021

Prom test case is failing, not sure if it is related.

Copy link
Contributor

@alolita alolita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. We will merge for next release.


exporters:
awsemf/application:
namespace: ECS/AWSOTel/Application
log_group_name: '/aws/ecs/application/metrics'
resource_to_telemetry_conversion:
enabled: true
dimension_rollup_option: NoDimensionRollup
metric_declarations:
- dimensions: [ [ TaskDefinitionFamily ] ]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe without this allow list, this will cause every resource dimension to be sent, which could be dozens, and likely fail to ingest to CW or otherwise cost too much. Don't we need to add a reasonable allow list here? I heard that TaskDefinitionFamily may not be sufficient, in that case what do we need and can we add them all here in the allow list instead of removing it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes @anuraaga. I am working on a new config. Discussing offline. Planning to allow a specific set of dimensions which are truly useful for ECS customers.

@jefchien
Copy link
Member

Prom test case is failing, not sure if it is related.

This was an issue I saw when refactoring the mock servers even before that. Investigating the issue: aws-observability/aws-otel-test-framework#404

@hossain-rayhan
Copy link
Contributor Author

Do not merge this. I am testing a new set of dimnsions.

Signed-off-by: Rayhan Hossain <hossain.rayhan@outlook.com>
@genbit
Copy link
Contributor

genbit commented Nov 17, 2021

Do I understand config correctly, this will export the following dimensions:

TaskDefinitionFamily
TaskDefinitionRevision
InstanceId
TaskARN
LaunchType
ClusterARN

@hossain-rayhan
Copy link
Contributor Author

Hi @genbit, We didn’t choose any dimension rollup option in the config. So it will use the default setup for awsemf exporter. We should see all attributes as single dimensions and another dimension with all attributes. I think thats the expected behavior for CW custoemrs as it's the default behavior in the exporter. The output should look like the following one.

                "Namespace": "ECS/AWSOTel/Application",
                "Dimensions": [
                    [
                        "TaskDefinitionFamily",
                        "TaskDefinitionRevision",
                        "LaunchType",
                        "TaskARN",
                        "ClusterARN"
                    ],
                    [],
                    [
                        "TaskDefinitionRevision"
                    ],
                    [
                        "LaunchType"
                    ],
                    [
                        "TaskDefinitionFamily"
                    ],
                    [
                        "ClusterARN"
                    ],
                    [
                        "TaskARN"
                    ]
                ],

Signed-off-by: Rayhan Hossain <hossain.rayhan@outlook.com>
Copy link
Contributor

@alolita alolita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm.

@alolita alolita merged commit 36226a8 into aws-observability:main Nov 17, 2021
jefchien pushed a commit that referenced this pull request Nov 17, 2021
* Update ecs default config for CloudWatch

Signed-off-by: Rayhan Hossain <hossain.rayhan@outlook.com>

* Select proper dimensions for ECS customers

Signed-off-by: Rayhan Hossain <hossain.rayhan@outlook.com>

* add NoDimensionRollUp for awsemf exporter

Signed-off-by: Rayhan Hossain <hossain.rayhan@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants