You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
When reviewing #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 (49ae09a) 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: #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 49ae09a 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:
Follows the same design pattern as any other standard module in beats.
Comparatively less expensive operations i.e., every Fetch call now uses the same configuration initialized once before and it is not done repeatedly for every Fetch. So, there would be lesser resource consumption.
Some naming fixes.
Lesser code and easier maintainability, etc.
Here's a prototype of the suggested solution: ritalwar#1
The suggested solution was made for #34624 but we later decided that it should go in a separate pull request. After #34624 is merged, I'll create a new PR with changes similar to the proposed solution.
Describe the enhancement:
When reviewing #34624 I came across a bug which was essentially due to improper propagation of configuration i.e.,
clientOptions
wasn't retaining the hosts. ThisclientOptions
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 (49ae09a) 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: #7401 and it was really a good approach. The configuration passing was proper i.e., there is a field
DialInfo
in the struct namedMetricSet
which is holding the config state properly and passing it properly. The proposed solution for #35502 is similar.Changes done in 49ae09a made the module metric fetching unnecessarily expensive. Previously one-time operations were done in
NewMetricSet
and not again and again in theNewClient
method (Fetch
callsNewClient
).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:Fetch
call now uses the same configuration initialized once before and it is not done repeatedly for everyFetch
. So, there would be lesser resource consumption.Here's a prototype of the suggested solution: ritalwar#1
The suggested solution was made for #34624 but we later decided that it should go in a separate pull request. After #34624 is merged, I'll create a new PR with changes similar to the proposed solution.
cc: @ritalwar @lalit-satapathy
The text was updated successfully, but these errors were encountered: