elastic / beats

:tropical_fish: Beats - Lightweight shippers for Elasticsearch & Logstash
https://www.elastic.co/products/beats
Other
12.14k stars 4.91k forks source link

metricbeat/module/mongodb: Refactor mongodb module #35502

Open shmsr opened 1 year ago

shmsr commented 1 year ago

Describe the enhancement:

When reviewing https://github.com/elastic/beats/pull/34624 I came across a bug which was essentially due to improper propagation of configuration i.e., clientOptions wasn't retaining the hosts. This clientOptions is used by mongo client to establish the connection and get the desired metrics.

Upon debugging the same with @ritalwar we found out the commit (49ae09a5614b79696fd02e752bee56492a9ca72d) that involved some major refactoring which led to the same. On further digging, I found that it is not aligned with other modules in beats repository (e.g., aerospike, redis, etc.). Before that commit it was fine and even along the same lines as other modules. The immediate parent of that commit reveals that the structure was more or less fine before that. Here's a PR made long back that introduced a common builder pattern: https://github.com/elastic/beats/pull/7401 and it was really a good approach. The configuration passing was proper i.e., there is a field DialInfo in the struct named MetricSet which is holding the config state properly and passing it properly. The proposed solution for #35502 is similar.

Changes done in 49ae09a5614b79696fd02e752bee56492a9ca72d made the module metric fetching unnecessarily expensive. Previously one-time operations were done in NewMetricSet and not again and again in the NewClient method (Fetch calls NewClient).

In order to bring back proper propagation of clientOptions, it is important to do some refactoring. With the suggested refactoring, we would the following improvements:

Here's a prototype of the suggested solution: https://github.com/ritalwar/beats/pull/1

The suggested solution was made for https://github.com/elastic/beats/pull/34624 but we later decided that it should go in a separate pull request. After https://github.com/elastic/beats/pull/34624 is merged, I'll create a new PR with changes similar to the proposed solution.

cc: @ritalwar @lalit-satapathy

elasticmachine commented 1 year ago

Pinging @elastic/integrations (Team:Integrations)