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 Cloudwatch] Add metric_type #6999

Merged

Conversation

tetianakravchenko
Copy link
Contributor

@tetianakravchenko tetianakravchenko commented Jul 18, 2023

What does this PR do?

Add metric_type for the aws cloudwatch data_stream.

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

Related issues

Screenshots

After this change:

  1. Will be added dynamic template mapping:
Screenshot 2023-07-18 at 15 42 14 2. will be removed: Screenshot 2023-07-18 at 15 42 36
  1. After the rollover time_series_metric will be added to the mapping:
Screenshot 2023-07-18 at 16 25 20

Note: for some reason not all metrics have

{
    "type": "double",
     "time_series_metric": "gauge"
},

Asked Fleet team to check the mapping generations

Why can't be used

- name: 'aws.*.metrics.*.*'
  type: object
  metric_type: gauge
  description: |
    Metrics that returned from Cloudwatch API query.

?
time_series_metric will not be added for this case.

Why the format was changed from:

- name: aws
  type: group
  fields:
  - name: '*.metrics.*.*'
     type: object
     metric_type: gauge
     description: |
        Metrics that returned from Cloudwatch API query.

to:

- name: 'aws.*.metrics.*.*'
  type: float
  metric_type: gauge
  description: |
    Metrics that returned from Cloudwatch API query.

due to this limitation - elastic/kibana#162041

Signed-off-by: Tetiana Kravchenko <[email protected]>
Signed-off-by: Tetiana Kravchenko <[email protected]>
Signed-off-by: Tetiana Kravchenko <[email protected]>
description: |
Metrics that returned from Cloudwatch API query.
- name: 'aws.*.metrics.*.*'
type: double
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kaiyan-sheng could you please help here? usually type: object is used in combination with object_type, but here it is skipped, is it intentional?

Is it fine to define it as a double?

I've also checked beats - https://github.com/elastic/beats/blob/main/x-pack/metricbeat/module/aws/cloudwatch/_meta/fields.yml and can't find this field there

Copy link
Contributor

Choose a reason for hiding this comment

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

In beats, it's defined under aws: https://github.com/elastic/beats/blob/main/x-pack/metricbeat/module/aws/_meta/fields.yml#L27

You are right, we are missing the object type here, it should be something like:

      type: object
      object_type: double
      object_type_mapping_type: "*"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, I didn't look into this file. So it actually should be correct to set it to double, before this change - the object type was defined dynamically.

@elasticmachine
Copy link

elasticmachine commented Jul 18, 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-07-25T09:36:01.926+0000

  • Duration: 53 min 11 sec

Test stats 🧪

Test Results
Failed 0
Passed 206
Skipped 4
Total 210

🤖 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 Jul 18, 2023

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (17/17) 💚
Files 94.444% (17/18) 👎 -5.556
Classes 94.444% (17/18) 👎 -5.556
Methods 85.953% (257/299) 👎 -9.109
Lines 86.011% (7501/8721) 👍 6.165
Conditionals 100.0% (0/0) 💚

@tetianakravchenko tetianakravchenko marked this pull request as ready for review July 19, 2023 07:36
@tetianakravchenko tetianakravchenko requested a review from a team as a code owner July 19, 2023 07:36
license: basic
description: Collect logs and metrics from Amazon Web Services (AWS) with Elastic Agent.
type: integration
categories:
- aws
release: ga
conditions:
kibana.version: "^8.8.1"
kibana.version: "^8.9.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I probably missed something, why do we need to update the kibana version here? If the upgrade is required, then we have to wait till 8.9.0 is released before merging this PR then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've reverted this change for this PR - It will be added with the TSDB enablement in next PR.
As I understand 8.9.0 is planned for today anyway

@tetianakravchenko tetianakravchenko merged commit cd46a74 into elastic:main Jul 26, 2023
@elasticmachine
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants