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

[Metricbeat] azure: move event report into loop validDim loop #29945

Merged

Conversation

ClumsyPotato
Copy link
Contributor

@ClumsyPotato ClumsyPotato commented Jan 21, 2022

What does this PR do?

Moved the report.Event() function into the loop that iterates over all grouped Dimension in order to report all events instead of just a single one

Why is it important?

Trying to fix the issue in #27226
If a azure metric endpoint with multiple dimensions is pulled only one dimension is send back to elastic

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.

Related issues

Closes #27226

Also relates to this
https://support.elastic.co/cases/5008X000023agMSQAY

###Backport
Pls backport this to version 7.x.x

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jan 21, 2022
@cla-checker-service
Copy link

cla-checker-service bot commented Jan 21, 2022

💚 CLA has been signed

@mergify
Copy link
Contributor

mergify bot commented Jan 21, 2022

This pull request does not have a backport label. Could you fix it @ClumsyPotato? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v./d./d./d is the label to automatically backport to the 7./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

@mergify mergify bot added the backport-skip Skip notification from the automated backport with mergify label Jan 21, 2022
@elasticmachine
Copy link
Collaborator

elasticmachine commented Jan 21, 2022

💚 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: 2022-02-11T20:29:17.507+0000

  • Duration: 90 min 40 sec

Test stats 🧪

Test Results
Failed 0
Passed 2774
Skipped 240
Total 3014

💚 Flaky test report

Tests succeeded.

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

@ClumsyPotato ClumsyPotato marked this pull request as ready for review January 21, 2022 15:13
@ClumsyPotato ClumsyPotato reopened this Jan 21, 2022
@ClumsyPotato
Copy link
Contributor Author

/test

1 similar comment
@ClumsyPotato
Copy link
Contributor Author

/test

@ClumsyPotato ClumsyPotato force-pushed the azure-module-fetch-multi-dem-values branch from 7a43c53 to 0b0a1be Compare January 21, 2022 15:32
@ClumsyPotato ClumsyPotato changed the title move event report into loop validDim loop [Metricbeat] move event report into loop validDim loop Jan 24, 2022
@ClumsyPotato ClumsyPotato changed the title [Metricbeat] move event report into loop validDim loop [Metricbeat] azure: move event report into loop validDim loop Jan 24, 2022
@mtojek mtojek added the Team:Integrations Label for the Integrations team label Jan 24, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations (Team:Integrations)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jan 24, 2022
@kaiyan-sheng kaiyan-sheng self-assigned this Jan 24, 2022
@kaiyan-sheng
Copy link
Contributor

@ClumsyPotato Thank you so much for your contribution!!

@kaiyan-sheng
Copy link
Contributor

/test

@ClumsyPotato
Copy link
Contributor Author

@kaiyan-sheng Glad i can help. I am open for suggestion and changes :)

Can you also make sure that this fix gets backportet to version 7.x? That would be awsome :3

@kaiyan-sheng
Copy link
Contributor

@ClumsyPotato Sorry for the delay on reviewing this PR! Overall it looks good, one nit is seems like there are several lines of code are repeated:

event, metricList = createEvent(timestamp, defaultMetric, resource, groupTimeValues)
if client.Config.AddCloudMetadata {
	vm = client.GetVMForMetaData(&resource, groupTimeValues)
	addCloudVMMetadata(&event, vm, resource.Subscription)
}
if client.Config.DefaultResourceType == "" {
	event.ModuleFields.Put("metrics", metricList)
} else {
	for key, metric := range metricList {
		event.MetricSetFields.Put(key, metric)
	}
}

Could you refactor this a bit please? Also it would be good to run TestData to regenerate data.json sample file to show that multiple dimensions are all collected. If you can add some test maybe in data_test.go, that would be great!
Thanks!!

@Patrick-Eichhorn
Copy link
Contributor

/test

@ClumsyPotato
Copy link
Contributor Author

@kaiyan-sheng thanks for the input. We just moved the whole block into its own function. Hope thats fine now^^
About The integration tests...I did some changes to the test and tried to run the tests with mage IntegTest and GoIntegTest but it did not make any change to data.json
Any idea what could be the problem?

@kaiyan-sheng kaiyan-sheng added backport-v8.0.0 Automated backport with mergify and removed backport-skip Skip notification from the automated backport with mergify labels Feb 2, 2022
@kaiyan-sheng
Copy link
Contributor

@ClumsyPotato Thanks for the change! What I usually do to regenerate data.json file is: (using compute VM as an example) in azure/compute_vm/compute_vm_integration_test.go, remove //go:build integration && azure and // +build integration,azure. Then add all the environment variables required for testing: AZURE_CLIENT_ID, AZURE_CLIENT_SECRET and etc. Then change the DataFlag to true under metricbeat/mb/testing/flags/flags.go. After that you should be able to see updated data.json when running the TestData function.

This is probably not the right way to run the test but it works 😂

@endorama
Copy link
Member

endorama commented Feb 7, 2022

Should be possible to run TestData tests responsible for updating sample data by running, from within the metricset folder:

$ go test --tags integration,azure -data -run "TestData"

Note the -data flag and the tags (integrations is required, azure is required for this particular metricset). I've used this command for the GCP module successfully. -run "TestData" may be omitted, as it's only used to target data tests only.

A note about tags: the -data flag is a custom flag added by metricbeat framework. I will not be present in case tags do not match, as the relevant code will not be run and silently skipped (without the tag the test file is ignored by go compiler so the framework doesn't load). This may happen if there are different tags in the build tags of the metricset under test (i.e. the GCP billing module has also billing tag).

I don't have an Azure environment setup to test this PR right now, but if you can try this command and confirm it works I'll add it to the developer documentation :)

@kaiyan-sheng
Copy link
Contributor

/test

@kaiyan-sheng kaiyan-sheng added the backport-7.17 Automated backport to the 7.17 branch with mergify label Feb 7, 2022
@ClumsyPotato ClumsyPotato force-pushed the azure-module-fetch-multi-dem-values branch from c7e917e to 14323e9 Compare February 11, 2022 19:31
@ClumsyPotato
Copy link
Contributor Author

Hey @shelly-yao @endorama
I added a test that makes sure that a certain dimension is always present in the metricset.
Also running the test with the labels added like @endorama describe also works when running inside the monitor directory

@kaiyan-sheng
Copy link
Contributor

/test

@Patrick-Eichhorn
Copy link
Contributor

Hello @kaiyan-sheng,
I saw that there are backport labels for 7.17.0 and 8.0.0. Will be the bug-fix merged in older versions too? For example 7.13.X and above?

@kaiyan-sheng kaiyan-sheng added the backport-v8.1.0 Automated backport with mergify label Feb 14, 2022
@kaiyan-sheng
Copy link
Contributor

Hey @Patrick-Eichhorn, sorry we won't be able to backport it to 7.13.x because we are not planning to release any new patch release for 7.13 anymore.

@kaiyan-sheng kaiyan-sheng merged commit ef7a0b9 into elastic:main Feb 14, 2022
mergify bot pushed a commit that referenced this pull request Feb 14, 2022
Co-authored-by: Adrian Tomalla <[email protected]>
Co-authored-by: Patrick Eichhorn <[email protected]>
(cherry picked from commit ef7a0b9)
mergify bot pushed a commit that referenced this pull request Feb 14, 2022
Co-authored-by: Adrian Tomalla <[email protected]>
Co-authored-by: Patrick Eichhorn <[email protected]>
(cherry picked from commit ef7a0b9)
mergify bot pushed a commit that referenced this pull request Feb 14, 2022
Co-authored-by: Adrian Tomalla <[email protected]>
Co-authored-by: Patrick Eichhorn <[email protected]>
(cherry picked from commit ef7a0b9)
kaiyan-sheng pushed a commit that referenced this pull request Feb 14, 2022
#30380)

Co-authored-by: Adrian Tomalla <[email protected]>
Co-authored-by: Patrick Eichhorn <[email protected]>
(cherry picked from commit ef7a0b9)

Co-authored-by: ClumsyPotato <[email protected]>
kaiyan-sheng pushed a commit that referenced this pull request Feb 14, 2022
#30379)

Co-authored-by: Adrian Tomalla <[email protected]>
Co-authored-by: Patrick Eichhorn <[email protected]>
(cherry picked from commit ef7a0b9)

Co-authored-by: ClumsyPotato <[email protected]>
kaiyan-sheng pushed a commit that referenced this pull request Feb 15, 2022
#30381)

Co-authored-by: Adrian Tomalla <[email protected]>
Co-authored-by: Patrick Eichhorn <[email protected]>
(cherry picked from commit ef7a0b9)

Co-authored-by: ClumsyPotato <[email protected]>
v1v added a commit to v1v/beats that referenced this pull request Feb 21, 2022
…into feature/use-with-kind-k8s-env

* 'feature/use-with-kind-k8s-env' of github.com:v1v/beats: (52 commits)
  ci: home is declared within withBeatsEnv
  ci: use withKindEnv step
  ci: use getBranchesFromAliases and support next-patch-8 (elastic#30400)
  Update fields.yml (elastic#29609)
  Heartbeat: fix browser metrics and trace mappings (elastic#30258)
  Apply light edits to 8.0 changelog (elastic#30351)
  packetbeat/beater: make sure Npcap installation runs before interfaces are needed (elastic#30396)
  Add a ring-buffer reporter to libbeat (elastic#28750)
  Osquerybeat: Add install verification for osquerybeat (elastic#30388)
  update windows matrix support (elastic#30373)
  Refactor of metricbeat process-gathering metrics and system/process (elastic#30076)
  adjust next changelog wording (elastic#30371)
  [Metricbeat] azure: move event report into loop validDim loop (elastic#29945)
  fix: report GitHub Check before the cache (elastic#30372)
  Add support for non-unique keys in Kafka output headers (elastic#30369)
  ci: 6 major branch reached EOL (elastic#30357)
  reduce Elastic Agent shut down time by stopping processes concurrently (elastic#29650)
  [Filebeat] Add message to register encode/decode debug logs (elastic#30271)
  [libbeat] kafka message header support (elastic#29940)
  Heartbeat: set duration to zero for syntax errors (elastic#30227)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-7.17 Automated backport to the 7.17 branch with mergify backport-v8.0.0 Automated backport with mergify backport-v8.1.0 Automated backport with mergify bug Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Metricbeat] Azure module - Simultaneously fetch multi-dimensional metric values
6 participants