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][EC2] Add processors to rename and drop fields #5812

Closed
wants to merge 5 commits into from

Conversation

constanca-m
Copy link
Contributor

@constanca-m constanca-m commented Apr 6, 2023

What does this PR do?

This PR syncs the EC2 fields from integrations with the ones in metricbeat.

In metricbeat, there are processors that rename and drop fields. Example: aws.ec2.metrics.CPUCreditUsage.avg is renamed to
aws.ec2.cpu.credit_usage (code here). However, there were no such processors in the integration, which caused the values from the ec2 group to be empty.

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.

How to test this PR locally

  1. Go to AWS integration and run elastic-package build.
  2. Update the elastic-package registry with elastic-package stack up -v -d --services package-registry.

@constanca-m constanca-m requested a review from a team as a code owner April 6, 2023 13:44
Signed-off-by: constanca-m <[email protected]>
@constanca-m constanca-m self-assigned this Apr 6, 2023
@constanca-m constanca-m added Integration:aws AWS Team:Cloud-Monitoring Label for the Cloud Monitoring team labels Apr 6, 2023
@elasticmachine
Copy link

elasticmachine commented Apr 6, 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-06-30T09:19:24.771+0000

  • Duration: 52 min 6 sec

Test stats 🧪

Test Results
Failed 0
Passed 195
Skipped 4
Total 199

🤖 GitHub comments

Expand to view the GitHub comments

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

  • /test : Re-trigger the build.

Signed-off-by: constanca-m <[email protected]>
@elasticmachine
Copy link

elasticmachine commented Apr 6, 2023

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (16/16) 💚
Files 94.118% (16/17) 👎 -5.882
Classes 94.118% (16/17) 👎 -5.882
Methods 85.816% (242/282) 👍 25.056
Lines 86.044% (7460/8670) 👎 -13.492
Conditionals 100.0% (0/0) 💚

- remove:
ignore_missing: true
field:
- aws.ec2.metrics
Copy link
Contributor

Choose a reason for hiding this comment

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

do you think this could be considered a breaking change for customers who have potentially built dashboards on the existing (un-renamed) metrics fields?

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, regarding the metrics.* maybe it is a breaking change, but it is not in sync with beats. And the documentation is not correct because the fields are mixed up. But maybe the best option is to support both for now. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeh it would avoid worrying about breaking things, but for all other customers means a bunch of duplication which sucks. @vinaychandrasekhar @SubhrataK what do you think? so long as the dashboards still work can we remove fields? the public documentation does mention the fields in the exported fields table, as shown below.

Screenshot 2023-04-12 at 11 33 21

@constanca-m i also added a pipeline test to validate whatever decision we make here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Just my two cents.

do you think this could be considered a breaking change for customers who have potentially built dashboards on the existing (un-renamed) metrics fields?

It might break the dashboards, but I do not think it should be considered a breaking change. Users have to explicitly upgrade the integration, no functionality breaks automatically. We had similar cases in the past (example).

In any case, we can consider bump the version to 1.5.0 or even 2.0.0 to signal the field naming change, if anything.

But maybe the best option is to support both for now. What do you think?

This is a possible option, but it would only delay the inevitable (e.g removing aws.ec2.metrics fields). I would opt for this only we are really on a tight schedule, to unblock this PR.

@botelastic
Copy link

botelastic bot commented May 28, 2023

Hi! We just realized that we haven't looked into this PR in a while. We're sorry! We're labeling this issue as Stale to make it hit our filters and make sure we get back to it as soon as possible. In the meantime, it'd be extremely helpful if you could take a look at it as well and confirm its relevance. A simple comment with a nice emoji will be enough :+1. Thank you for your contribution!

@botelastic botelastic bot added the Stalled label May 28, 2023
@botelastic botelastic bot removed the Stalled label Jun 22, 2023
@girodav
Copy link
Contributor

girodav commented Jun 22, 2023

@constanca-m I see no change made on the OOTB dashboards. Have you verified that they work properly or they need to be updated?

@constanca-m
Copy link
Contributor Author

It works for me as well @girodav

@constanca-m
Copy link
Contributor Author

/test

@kaiyan-sheng
Copy link
Contributor

After discussion with @constanca-m and @tommyers-elastic , we decide to not rename these fields to match Metricbeat at this point in AWS integration. Instead we want to keep the name in the format from cloudwatch input and will change the fields.yml to make sure the field names are defined.

@constanca-m constanca-m closed this Jul 3, 2023
@constanca-m constanca-m deleted the add-ec2-processor branch July 19, 2023 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Integration:aws AWS Team:Cloud-Monitoring Label for the Cloud Monitoring team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants