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 mongodb multiple hosts connection issue #34624

Merged
merged 30 commits into from
May 24, 2023

Conversation

ritalwar
Copy link
Contributor

@ritalwar ritalwar commented Feb 21, 2023

  • Bug

What does this PR do?

This PR resolves the issues when multiple hosts are used for connection.
This will allow mongodb replica sets in connection string URI to successfully parse and fetch data.

Different host inputs scenarios tested as part of this PR are as follows:

  1. Multiple Hosts: mongodb://localhost:27017,localhost:27022,localhost:27023
  2. With DirectConnection option: mongodb://localhost:27017/?directConnection=true
  3. With Replica Set Option: mongodb://localhost:27017,localhost:27022,localhost:27023/?replicaSet=dbrs
  4. Single Host without mongodb prefix: localhost:27017
  5. With mongodb prefix: mongodb://localhost:27017
  6. "mongodb://myuser:mypass@localhost:40001", "otherhost:40001"

Why is it important?

Without this there will be a bug while using multiple hosts in mongodb connection URI.

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.

How to test this PR locally

Enable mongodb module. In mongodb.yml file test with both single and multiple hosts connections and it should work for both and fetch data successfully.

Related issues

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Feb 21, 2023
@mergify
Copy link
Contributor

mergify bot commented Feb 21, 2023

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

@elasticmachine
Copy link
Collaborator

elasticmachine commented Feb 21, 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-05-23T15:10:38.947+0000

  • Duration: 53 min 30 sec

Test stats 🧪

Test Results
Failed 0
Passed 4376
Skipped 890
Total 5266

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

@mergify
Copy link
Contributor

mergify bot commented Feb 23, 2023

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

@ritalwar ritalwar marked this pull request as ready for review February 23, 2023 09:33
@ritalwar ritalwar requested a review from a team as a code owner February 23, 2023 09:33
@lalit-satapathy lalit-satapathy added the Team:Service-Integrations Label for the Service Integrations team label Feb 28, 2023
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Feb 28, 2023
@lalit-satapathy
Copy link
Contributor

We can improve the documentation to capture different URL scenarios (single, replica set) supported?

@ritalwar
Copy link
Contributor Author

We can improve the documentation to capture different URL scenarios (single, replica set) supported?

Done.

@lalit-satapathy
Copy link
Contributor

@ritalwar, Can we also add the different MongoDB hosts configuration, currently testing has been done in the PR description?

@ritalwar
Copy link
Contributor Author

@ritalwar, Can we also add the different MongoDB hosts configuration, currently testing has been done in the PR description?

sure, will update.

@ritalwar ritalwar requested a review from shmsr April 17, 2023 11:40
Copy link
Member

@shmsr shmsr left a comment

Choose a reason for hiding this comment

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

Left some comments. Also, give me some more time to test these changes.

metricbeat/docs/modules/mongodb.asciidoc Outdated Show resolved Hide resolved
metricbeat/module/mongodb/mongodb_test.go Show resolved Hide resolved
metricbeat/module/mongodb/mongodb.go Show resolved Hide resolved
metricbeat/docs/modules/mongodb.asciidoc Show resolved Hide resolved
metricbeat/module/mongodb/mongodb_test.go Outdated Show resolved Hide resolved
@ritalwar
Copy link
Contributor Author

/test

@ritalwar
Copy link
Contributor Author

As per discussions with @ritalwar, I have opened a PR: ritalwar#1 (read the PR's description for more details) that aims to refactor mongodb module so that it aligns with standards of other modules. Changes also fixes the issues which we found during testing. In case the changes look good, we can cherry-pick the commit(s).

After further discussions with @lalit-satapathy, I'm currently addressing the issues identified during testing in this PR. To ensure better tracking and organization, we can handle the refactoring of the MongoDB module in a separate PR.

@ritalwar ritalwar requested a review from shmsr May 22, 2023 04:39
metricbeat/docs/modules/mongodb.asciidoc Outdated Show resolved Hide resolved
metricbeat/module/mongodb/_meta/docs.asciidoc Outdated Show resolved Hide resolved
@miltonhultgren miltonhultgren removed the request for review from a team May 22, 2023 14:28
Copy link
Contributor

@leehinman leehinman left a comment

Choose a reason for hiding this comment

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

code changes look good. just want a healthcheck in the docker-compose.yml to improve CI reliability

metricbeat/module/mongodb/docker-compose.yml Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Automation Label for the Observability productivity team Team:Service-Integrations Label for the Service Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MongoDB metricsets incorrectly handling multiple hosts
6 participants