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

[Oracle Module] Refactor module to properly use host parsers instead of doing its own parsing of hosts #31692

Merged
merged 14 commits into from
Jun 1, 2022

Conversation

yug-rajani
Copy link
Contributor

@yug-rajani yug-rajani commented May 20, 2022

  • Enhancement

What does this PR do?

Refactors the Oracle module to properly use host parsers instead of doing its own parsing of hosts.

Why is it important?

Metricbeat should instantiate one metricset per host, and the metricset shouldn't need to do any special handling to support multiple hosts. Using HostData is the only way a metricset instance has to know what host to use when multiple hosts are configured. When parsing the configured host, it allows to set in the HostData the Host, that is later used for service.address. It abstracts the handling of multiple hosts, each metricset instance receives a single HostData with the host they have to use.

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

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label May 20, 2022
@yug-rajani yug-rajani self-assigned this May 20, 2022
@elasticmachine
Copy link
Collaborator

elasticmachine commented May 20, 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-06-01T02:16:47.516+0000

  • Duration: 59 min 41 sec

Test stats 🧪

Test Results
Failed 0
Passed 1068
Skipped 91
Total 1159

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

@yug-rajani yug-rajani requested a review from kush-elastic May 20, 2022 09:30
Copy link
Collaborator

@kush-elastic kush-elastic left a comment

Choose a reason for hiding this comment

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

Just a small comment.
Rest LGTM!
Please wait for the others approvals.

x-pack/metricbeat/module/oracle/connection.go Outdated Show resolved Hide resolved
@yug-rajani yug-rajani added the Team:Service-Integrations Label for the Service Integrations team label May 23, 2022
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label May 23, 2022
@yug-rajani yug-rajani marked this pull request as ready for review May 23, 2022 13:28
@yug-rajani yug-rajani requested a review from a team as a code owner May 23, 2022 13:28
@yug-rajani yug-rajani requested review from jsoriano and agithomas May 23, 2022 13:29
@mergify
Copy link
Contributor

mergify bot commented May 23, 2022

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 oracle_refactor_host_parsing upstream/oracle_refactor_host_parsing
git merge upstream/main
git push upstream oracle_refactor_host_parsing

@mergify
Copy link
Contributor

mergify bot commented May 24, 2022

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 oracle_refactor_host_parsing upstream/oracle_refactor_host_parsing
git merge upstream/main
git push upstream oracle_refactor_host_parsing

@mergify
Copy link
Contributor

mergify bot commented May 24, 2022

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 oracle_refactor_host_parsing upstream/oracle_refactor_host_parsing
git merge upstream/main
git push upstream oracle_refactor_host_parsing

@agithomas
Copy link
Contributor

Please revisit the testing.go.
Please refer other modules such as mysql / cockroachdb.

@yug-rajani
Copy link
Contributor Author

yug-rajani commented May 25, 2022

Please revisit the testing.go. Please refer other modules such as mysql / cockroachdb.

As a part of this PR, we are refactoring the host parsing mechanism of the Oracle Module, and based on our analysis, there does not seem to be a change required in the tests with respect to the changes we have made in the module. The testing.go (link) file of the module was developed along with the previous implementation (in which a single host is being passed as expected) and the same tests are passing with respect to the changes in this PR.

@mergify
Copy link
Contributor

mergify bot commented May 25, 2022

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 oracle_refactor_host_parsing upstream/oracle_refactor_host_parsing
git merge upstream/main
git push upstream oracle_refactor_host_parsing

Copy link
Contributor

@agithomas agithomas left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@agithomas agithomas requested a review from mtojek May 27, 2022 05:00
CHANGELOG.next.asciidoc Outdated Show resolved Hide resolved
@yug-rajani yug-rajani requested a review from ishleenk17 June 1, 2022 02:23
@agithomas agithomas merged commit 3c7cc8c into elastic:main Jun 1, 2022
chrisberkhout pushed a commit that referenced this pull request Jun 1, 2023
…of doing its own parsing of hosts (#31692)

* Update host parsing mechanism

* Update comment to add more description

* Add CHANGELOG.next.asciidoc entry

* Update CHANGELOG.next.asciidoc to remove accidentally added PRs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Service-Integrations Label for the Service Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Oracle Module] Refactor module to properly use host parsers instead of doing its own parsing of hosts
5 participants