beats icon indicating copy to clipboard operation
beats copied to clipboard

fix: Fix parsing for dimension names from CloudWatch tag identifier values

Open cjr125 opened this issue 3 years ago • 6 comments
trafficstars

  • Bug fix

What does this PR do?

Fix parsing for dimension names from CloudWatch tag identifier values

Why is it important?

This change is necessary for filtering CloudWatch metric tags with dimensions attached to identifier values

Checklist

  • [x] My code follows the style guidelines of this project
  • [x] I have commented my code, particularly in hard-to-understand areas
  • [x] I have made corresponding changes to the documentation
  • [x] I have made corresponding change to the default configuration files
  • [x] I have added tests that prove my fix is effective or that my feature works
  • [x] I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

  • [ ] Approach to remove dimension names from identifier values is correct

cjr125 avatar Sep 09 '22 16:09 cjr125

This pull request does not have a backport label. If this is a bug or security fix, could you label this PR @cjr125? 🙏. For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed branches, such as:

  • backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

mergify[bot] avatar Sep 09 '22 16:09 mergify[bot]

:grey_exclamation: Build Aborted

The PR is not allowed to run in the CI yet

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

Expand to view the summary

Build stats

  • Start Time: 2022-10-11T22:16:46.801+0000

  • Duration: 5 min 36 sec

Steps errors 2

Expand to view the steps failures

Load a resource file from a library
  • Took 0 min 0 sec . View more details here
  • Description: approval-list/elastic/beats.yml
Error signal
  • Took 0 min 0 sec . View more details here
  • Description: githubApiCall: The REST API call https://api.github.com/orgs/elastic/members/cjr125 return the message : java.lang.Exception: httpRequest: Failure connecting to the service https://api.github.com/orgs/elastic/members/cjr125 : httpRequest: Failure connecting to the service https://api.github.com/orgs/elastic/members/cjr125 : Code: 404Error: {"message":"User does not exist or is not a member of the organization","documentation_url":"https://docs.github.com/rest/reference/orgs#check-organization-membership-for-a-user"}

:robot: GitHub comments

Expand to view the GitHub comments

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

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

elasticmachine avatar Sep 09 '22 16:09 elasticmachine

This pull request is now in conflicts. Could you fix it? 🙏 To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b review/addCloudFront upstream/review/addCloudFront
git merge upstream/main
git push upstream review/addCloudFront

mergify[bot] avatar Sep 20 '22 21:09 mergify[bot]

@cjr125 there's no need to add a new metricset to metrcibeat

you can collect the metrics you need just adding the following to your modules.d/aws.yml:

- module: aws
  period: 5m
  metricsets:
    - cloudwatch
  metrics:
      - namespace: AWS/CloudFront
        resource_type: cloudfront
        statistic: ["Average"]
        name:
          - 4xxErrorRate
          - 401ErrorRate
          - 403ErrorRate
          - 404ErrorRate
          - 5xxErrorRate
          - 502ErrorRate
          - 503ErrorRate
          - 504ErrorRate
          - CacheHitRate
          - TotalErrorRate
          - ExecutionTime
      - namespace: AWS/CloudFront
        resource_type: cloudfront
        statistic: ["Sum"]
        name:
          - BytesDownloaded
          - BytesUploaded
          - Requests
          - Invocations
          - ValidationErrors
          - ExecutionErrors
          - FunctionThrottles
      - namespace: AWS/CloudFront
        resource_type: cloudfront
        statistic: ["p95"] # or others relevant pNN
        name:
          - OriginLatency

paolafrancesca avatar Sep 21 '22 01:09 paolafrancesca

💚 CLA has been signed

@cjr125 there's no need to add a new metricset to metrcibeat

you can collect the metrics you need just adding the following to your modules.d/aws.yml:

- module: aws
  period: 5m
  metricsets:
    - cloudwatch
  metrics:
      - namespace: AWS/CloudFront
        resource_type: cloudfront
        statistic: ["Average"]
        name:
          - 4xxErrorRate
          - 401ErrorRate
          - 403ErrorRate
          - 404ErrorRate
          - 5xxErrorRate
          - 502ErrorRate
          - 503ErrorRate
          - 504ErrorRate
          - CacheHitRate
          - TotalErrorRate
          - ExecutionTime
      - namespace: AWS/CloudFront
        resource_type: cloudfront
        statistic: ["Sum"]
        name:
          - BytesDownloaded
          - BytesUploaded
          - Requests
          - Invocations
          - ValidationErrors
          - ExecutionErrors
          - FunctionThrottles
      - namespace: AWS/CloudFront
        resource_type: cloudfront
        statistic: ["p95"] # or others relevant pNN
        name:
          - OriginLatency

@aspacca I do actually need part of this code to fix a problem with using tags_filter on CloudFront metrics in CloudWatch. The identifier values include dimension names which break the standard tag filtering logic. please see the updates (I left the commit history)

cjr125 avatar Oct 06 '22 12:10 cjr125

The identifier values include dimension names which break the standard tag filtering logic. please see the updates (I left the commit history)

can you explain your use case with an example? From what I see it looks like a breaking change, and if so we cannot merge without proper protocol

paolafrancesca avatar Oct 11 '22 01:10 paolafrancesca

This pull request is now in conflicts. Could you fix it? 🙏 To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b review/addCloudFront upstream/review/addCloudFront
git merge upstream/main
git push upstream review/addCloudFront

mergify[bot] avatar Oct 11 '22 03:10 mergify[bot]

The identifier values include dimension names which break the standard tag filtering logic. please see the updates (I left the commit history)

can you explain your use case with an example? From what I see it looks like a breaking change, and if so we cannot merge without proper protocol

When loading metrics using the following config, I am able to print the following:

  aws.yml: |-
    - module: aws
      period: 1m
      metricsets:
        - cloudwatch
      metrics:
        - namespace: AWS/CloudFront
          resource_type: cloudfront
          statistic: ["Average"]
          name:
            - 4xxErrorRate
            - 401ErrorRate
            - 403ErrorRate
            - 404ErrorRate
            - 5xxErrorRate
            - 502ErrorRate
            - 503ErrorRate
            - 504ErrorRate
            - CacheHitRate
            - TotalErrorRate
            - ExecutionTime
        - namespace: AWS/CloudFront
          resource_type: cloudfront
          statistic: ["Sum"]
          name:
            - BytesDownloaded
            - BytesUploaded
            - Requests
            - Invocations
            - ValidationErrors
            - ExecutionErrors
            - FunctionThrottles
        - namespace: AWS/CloudFront
          resource_type: cloudfront
          statistic: ["p95"] # or others relevant pNN
          name:
            - OriginLatency
      tags_filter:
        - key: "amwell:environment-name"
          value: "dev-next"
// remove dimension names from identifier value if applicable
if strings.Contains(identifierValue, dimensionSeparator) {
	fmt.Println("identifierValue:", identifierValue)
	identifierSeparators := strings.Split(identifierValue, dimensionSeparator)
	identifierValue = identifierSeparators[len(identifierSeparators)-1]
}

identifierValue: Global,E5TGO8HHLKEA8

cjr125 avatar Oct 11 '22 13:10 cjr125

identifierValue: Global,E5TGO8HHLKEA8

you should use this full value in the tags_filter

with the other PR you've created you can add multiple values for the same tag filter

paolafrancesca avatar Oct 12 '22 00:10 paolafrancesca

identifierValue: Global,E5TGO8HHLKEA8

you should use this full value in the tags_filter

with the other PR you've created you can add multiple values for the same tag filter

I'm not sure I follow what you are suggesting. These identifier values are what come back when I try to apply the filter for a specific application environment which we need to monitor. I need the filter to select all CloudFront distributions tagged with this environment name. For other namespaces, the identifier values do not contain dimension names (Global in this case) and the filtering works properly. In the case of the CloudFront namespace, however, the dimension name must be trimmed off in order to capture the desired identifier value. Is there some other way this problem can be approached?

cjr125 avatar Oct 12 '22 07:10 cjr125

This pull request is now in conflicts. Could you fix it? 🙏 To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b review/addCloudFront upstream/review/addCloudFront
git merge upstream/main
git push upstream review/addCloudFront

mergify[bot] avatar Oct 25 '22 09:10 mergify[bot]

@cjr125 Im trying to fix this issue in https://github.com/elastic/beats/pull/33472, please feel free to test/review it.

kaiyan-sheng avatar Oct 28 '22 21:10 kaiyan-sheng