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

[AWS Fargate] Remove memory.usage.pct field #8254

Merged

Conversation

lucian-ioan
Copy link
Contributor

@lucian-ioan lucian-ioan commented Oct 19, 2023

Proposed commit message

Remove memory.usage.pct field and use memory.usage.total instead for plain memory usage.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

Author's Checklist

How to test this PR locally

Blog post for setting up the integration with Elastic container sidecar.

Related issues

Screenshots

Before:
before

After:
aws

@lucian-ioan lucian-ioan self-assigned this Oct 19, 2023
@lucian-ioan lucian-ioan added Integration:AWS enhancement New feature or request labels Oct 19, 2023
@elasticmachine
Copy link

elasticmachine commented Oct 19, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-10-24T10:40:07.485+0000

  • Duration: 28 min 44 sec

Test stats 🧪

Test Results
Failed 0
Passed 3
Skipped 0
Total 3

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@elasticmachine
Copy link

elasticmachine commented Oct 22, 2023

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (0/0) 💚
Files 100.0% (0/0) 💚
Classes 100.0% (0/0) 💚
Methods 66.667% (2/3) 👎 -33.333
Lines 100.0% (0/0) 💚 4.172
Conditionals 100.0% (0/0) 💚

@lucian-ioan lucian-ioan mentioned this pull request Oct 22, 2023
14 tasks
@lucian-ioan lucian-ioan marked this pull request as ready for review October 22, 2023 21:43
@lucian-ioan lucian-ioan requested a review from a team as a code owner October 22, 2023 21:43
Copy link
Contributor

@zmoog zmoog left a comment

Choose a reason for hiding this comment

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

With this PR, we are removing the memory.usage.pct field and replacing it with the memory.usage.total to represent the memory usage in containers.

Deprecating usually refers to flagging a data field to suggest to users to avoid it because it will no longer be available in future releases.

Copy link
Contributor

@zmoog zmoog left a comment

Choose a reason for hiding this comment

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

I suggest to align the PR title and description, stating that it removes the memory.usage.pct field.

@lucian-ioan lucian-ioan changed the title [AWS Fargate] Deprecate memory.usage.pct field [AWS Fargate] Remove memory.usage.pct field Oct 24, 2023
Copy link
Contributor

@zmoog zmoog left a comment

Choose a reason for hiding this comment

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

LGTM.

Since this is a beta data stream, it seems okay to remove a field. WDYT @lalit-satapathy?

@agithomas
Copy link
Contributor

@lucian-ioan , as per the inputs mentioned in the SDH, it is best to have the below information represented as Metrics panel

  • Soft limit
  • Hard limit
  • Hierarchical limit

This is good considering we are removing the memory utilisation percentage information from the dashboard.

@lucian-ioan
Copy link
Contributor Author

lucian-ioan commented Nov 3, 2023

@agithomas Currently, we are not collecting some limits at input level which is why I chose not to include them.

Should I look into it and open a PR in beats? The issue will delay this PR and the AWS Fargate to GA quite a bit.

@agithomas
Copy link
Contributor

Should I look into it and open a PR in beats? The issue is this will delay this PR and the AWS Fargate to GA quite a bit.

From the SDH it is clear that the customers are looking for the memory utilisation information. So, it is highly recommend to have the limits collected and shown in the dashboard . This can be taken up after making GA as well, but must be taken up at the earliest.

Is there any immediate requirement to make AWS Fargate GA?

cc @tommyers-elastic , @lalit-satapathy

@lucian-ioan
Copy link
Contributor Author

lucian-ioan commented Nov 14, 2023

@lucian-ioan , as per the inputs mentioned in the SDH, it is best to have the below information represented as Metrics panel

  • Soft limit
  • Hard limit
  • Hierarchical limit

This is good considering we are removing the memory utilisation percentage information from the dashboard.

Retrieved task metadata using AWS CLI and the instructions in the official Amazon ECS documentation.

  1. Soft Limit - not reported by AWS
  2. Hard Limit is available via ${ECS_CONTAINER_METADATA_URI_V4}/task
    Limits": {
        "CPU": 1024,
        "Memory": 3840
      }
  1. Hierarchical Memory Limit is available via ${ECS_CONTAINER_METADATA_URI_V4}/task/stats

Beats issue, beats PR to follow shortly.

@botelastic botelastic bot added the Stalled label Dec 14, 2023
@mrodm
Copy link
Contributor

mrodm commented Dec 21, 2023

Hi @lucian-ioan, please update your branch with the latest contents from main branch. There was an important PR merged updating the CI pipelines. Thanks!

@botelastic botelastic bot removed the Stalled label Dec 21, 2023
@lucian-ioan lucian-ioan requested a review from a team as a code owner January 2, 2024 14:38
@elastic elastic deleted a comment from botelastic bot Jan 2, 2024
@lucian-ioan
Copy link
Contributor Author

Tested this with 8.12.0 and it will be ready for review as soon as 8.12.0 releases on the 9th of Jan.

Copy link
Contributor

@agithomas agithomas left a comment

Choose a reason for hiding this comment

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

LGTM!

@agithomas
Copy link
Contributor

@lucian-ioan , can we have this merged, based on your comment ?

@lucian-ioan
Copy link
Contributor Author

lucian-ioan commented Jan 17, 2024

Beats release schedule changed, testing now with the new 8.11.4 minor version which was released late last week.

@lucian-ioan
Copy link
Contributor Author

I think the memory limit makes the most sense in the table, WDYT @agithomas ?

screencapture-elastic-package-test-serverless-c10976-kb-us-east-1-aws-elastic-cloud-app-dashboards-2024-01-24-03_44_35

Opted for using container_id instead of the ECS name due to scenarios where the name could be the same for different containers.

@agithomas
Copy link
Contributor

I think the memory limit makes the most sense in the table, WDYT @agithomas ?

Please see my comment here

@lucian-ioan
Copy link
Contributor Author

lucian-ioan commented Jan 24, 2024

Since the solution proposed in the SDH, several observations:

  • Soft Memory Limit is not reported by the AWS API
  • Heirarchical Memory Limit is not container dependent and can be reported as an unusually high value
  • Hard Memory Limit is always reported correctly per container

There can be many different hard limits so a plain metrics counter wouldn't work in this case.

I'm thinking of two solutions:

  1. Table with columns for Max Container Memory Used and Container Memory Hard Limit
  2. Plotting the limits in the Container Memory Usage line chart to be able to compare them

@agithomas
Copy link
Contributor

2. Plotting the limits in the Container Memory Usage line chart to be able to compare them

This may not be needed. I am afraid, this would lead to the confusions that led to the original SDH.

@agithomas
Copy link
Contributor

  1. Table with columns for Max Container Memory Used and Container Memory Hard Limit

It it the one that is mentioned here in the screenshot ?

@lucian-ioan
Copy link
Contributor Author

lucian-ioan commented Jan 24, 2024

yes @agithomas, it could also be Latest Memory Usage, Max Memory Usage and Hard Memory Limit as columns.

WDYT @zmoog?

@lucian-ioan
Copy link
Contributor Author

lucian-ioan commented Feb 6, 2024

Made the changes discussed to the dashboard @agithomas, please let me know if they are ok:

awsfargate-integration-overview png

Found a way to convert the Memory Hard Limits in GB (as they are in AWS) using a formula.

@elasticmachine
Copy link

💚 Build Succeeded

History

cc @lucian-ioan

Copy link

Quality Gate passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No Coverage information No data about Coverage
No Duplication information No data about Duplication

See analysis details on SonarQube

@agithomas
Copy link
Contributor

LGTM!

@lucian-ioan lucian-ioan merged commit 6ce818d into elastic:main Feb 8, 2024
5 checks passed
@elasticmachine
Copy link

Package awsfargate - 0.5.0 containing this change is available at https://epr.elastic.co/search?package=awsfargate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Integration:AWS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants