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

Fix value mapping for min and max aggregation type in Azure Monitor #37643

Merged

Conversation

zmoog
Copy link
Contributor

@zmoog zmoog commented Jan 16, 2024

Proposed commit message

Fix mapping of metric values from Metric to KeyValuePoint. Unfortunately, I didn't notice the code for min and max aggregations picked up the value from the avg field. This is a silly mistake with nasty consequences.

I added a basic unit test to check the expected behavior.

Checklist

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

Author's Checklist

How to test this PR locally

Set up the Azure Monitor metricset with the following Metricbeat module configuration:

- module: azure
  metricsets:
  - monitor
  enabled: true
  period: 300s
  client_id: '...'
  client_secret: '...'
  tenant_id: '...'
  subscription_id: '...'
  refresh_list_interval: 600s
  resources:
  - resource_query: resourceType eq 'Microsoft.Cache/redis'
     metrics:
      - dimensions:
          - name: ShardId
            value: '*'
          - name: Port
            value: '*'
          - name: Primary
            value: '*'
        ignore_unsupported: true
        name:
          - allcachehits
          - allcachemisses
          - allcacheRead
          - allcacheWrite
          - allconnectedclients
          - allevictedkeys
          - allexpiredkeys
          - allgetcommands
          - alloperationsPerSecond
          - allpercentprocessortime
          - allserverLoad
          - allsetcommands
          - alltotalcommandsprocessed
          - alltotalkeys
          - allusedmemory
          - allusedmemorypercentage
          - allusedmemoryRss
          - ConnectedClientsUsingAADToken
        namespace: Microsoft.Cache/redis
        timegrain: PT1M

Run the metricset to collect some metrics.

The collected documents must contain metrics having min and max aggregation types (for example, azure.metrics.server_load.max).

Related issues

Screenshots

With the fix, I am able to successfully collect metric with max aggregation:

CleanShot 2024-01-16 at 17 17 35@2x

Metric values for `min` and `max` aggregation types were picked up
from the wrong place.
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jan 16, 2024
@zmoog zmoog added the Team:obs-ds-hosted-services Label for the Observability Hosted Services team label Jan 16, 2024
@mergify mergify bot assigned zmoog Jan 16, 2024
Copy link
Contributor

mergify bot commented Jan 16, 2024

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @zmoog? 🙏.
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

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jan 16, 2024
@zmoog zmoog added bug needs_team Indicates that the issue/PR needs a Team:* label labels Jan 16, 2024
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jan 16, 2024
@elasticmachine
Copy link
Collaborator

💚 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: 2024-01-16T12:30:43.777+0000

  • Duration: 55 min 10 sec

Test stats 🧪

Test Results
Failed 0
Passed 1550
Skipped 96
Total 1646

💚 Flaky test report

Tests succeeded.

🤖 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!)

@zmoog zmoog changed the title [Azure] Fix value mapping for min and max aggregation type in Azure Monitor Fix value mapping for min and max aggregation type in Azure Monitor Jan 16, 2024
@zmoog zmoog added backport-v8.11.0 Automated backport with mergify backport-v8.12.0 Automated backport with mergify labels Jan 16, 2024
@elasticmachine
Copy link
Collaborator

❕ Build Aborted

There is a new build on-going so the previous on-going builds have been aborted.

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: 2024-01-16T14:10:18.617+0000

  • Duration: 30 min 8 sec

Test stats 🧪

Test Results
Failed 0
Passed 588
Skipped 32
Total 620

Steps errors 1

Expand to view the steps failures

Error signal
  • Took 0 min 0 sec . View more details here
  • Description: Error 'org.jenkinsci.plugins.workflow.steps.FlowInterruptedException'

🤖 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!)

@zmoog zmoog marked this pull request as ready for review January 16, 2024 14:42
@zmoog zmoog requested a review from a team as a code owner January 16, 2024 14:43
@elasticmachine
Copy link
Collaborator

Pinging @elastic/obs-ds-hosted-services (Team:obs-ds-hosted-services)

CHANGELOG.next.asciidoc Outdated Show resolved Hide resolved
Co-authored-by: Andrew Gizas <andreas.gkizas@elastic.co>
@elasticmachine
Copy link
Collaborator

❕ Build Aborted

There is a new build on-going so the previous on-going builds have been aborted.

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: 2024-01-16T14:32:55.667+0000

  • Duration: 34 min 42 sec

Test stats 🧪

Test Results
Failed 0
Passed 1558
Skipped 96
Total 1654

Steps errors 2

Expand to view the steps failures

Cleanup
  • Took 0 min 0 sec . View more details here
  • Description: rm source.tgz
Error signal
  • Took 0 min 0 sec . View more details here
  • Description: untar: step failed with error null

🤖 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
Copy link
Collaborator

❕ Build Aborted

There is a new build on-going so the previous on-going builds have been aborted.

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: 2024-01-16T15:01:46.143+0000

  • Duration: 14 min 52 sec

Test stats 🧪

Test Results
Failed 0
Passed 3
Skipped 0
Total 3

Steps errors 1

Expand to view the steps failures

Error signal
  • Took 0 min 0 sec . View more details here
  • Description: Error 'org.jenkinsci.plugins.workflow.steps.FlowInterruptedException'

🤖 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
Copy link
Collaborator

💚 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

  • Duration: 51 min 48 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 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!)

@zmoog zmoog merged commit 1554d7d into elastic:main Jan 16, 2024
28 checks passed
@zmoog zmoog deleted the zmoog/fix-min-max-aggregations-in-azure-monitor branch January 16, 2024 16:26
mergify bot pushed a commit that referenced this pull request Jan 16, 2024
…37643)

* Fix value mapping for min and max aggregation type

Metric values for `min` and `max` aggregation types were picked up
from the wrong place.

---------

Co-authored-by: Andrew Gizas <andreas.gkizas@elastic.co>
(cherry picked from commit 1554d7d)
mergify bot pushed a commit that referenced this pull request Jan 16, 2024
…37643)

* Fix value mapping for min and max aggregation type

Metric values for `min` and `max` aggregation types were picked up
from the wrong place.

---------

Co-authored-by: Andrew Gizas <andreas.gkizas@elastic.co>
(cherry picked from commit 1554d7d)
zmoog added a commit that referenced this pull request Jan 16, 2024
…37643) (#37648)

* Fix value mapping for min and max aggregation type

Metric values for `min` and `max` aggregation types were picked up
from the wrong place.

---------

Co-authored-by: Andrew Gizas <andreas.gkizas@elastic.co>
(cherry picked from commit 1554d7d)

Co-authored-by: Maurizio Branca <maurizio.branca@elastic.co>
zmoog added a commit that referenced this pull request Jan 16, 2024
…37643)

* Fix value mapping for min and max aggregation type

Metric values for `min` and `max` aggregation types were picked up
from the wrong place.

---------

Co-authored-by: Andrew Gizas <andreas.gkizas@elastic.co>
(cherry picked from commit 1554d7d)
zmoog added a commit that referenced this pull request Jan 17, 2024
…37643)

* Fix value mapping for min and max aggregation type

Metric values for `min` and `max` aggregation types were picked up
from the wrong place.

---------

Co-authored-by: Andrew Gizas <andreas.gkizas@elastic.co>
(cherry picked from commit 1554d7d)
zmoog added a commit that referenced this pull request Jan 17, 2024
…37643) (#37649)

* Fix value mapping for min and max aggregation type

Metric values for `min` and `max` aggregation types were picked up
from the wrong place.

---------

Co-authored-by: Andrew Gizas <andreas.gkizas@elastic.co>
(cherry picked from commit 1554d7d)

Co-authored-by: Maurizio Branca <maurizio.branca@elastic.co>
Scholar-Li pushed a commit to Scholar-Li/beats that referenced this pull request Feb 5, 2024
…lastic#37643)

* Fix value mapping for min and max aggregation type

Metric values for `min` and `max` aggregation types were picked up
from the wrong place.

---------

Co-authored-by: Andrew Gizas <andreas.gkizas@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v8.11.0 Automated backport with mergify backport-v8.12.0 Automated backport with mergify bug Team:obs-ds-hosted-services Label for the Observability Hosted Services team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Azure Resource Metrics missing metrics after upgrade from 8.11.1 to 8.11.3
5 participants