Improvement/bb 614 remove redis metrics #2588

benzekrimaha · 2024-11-06T18:28:41Z

bert-e · 2024-11-06T18:28:45Z

Hello benzekrimaha,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Available options

name	description	privileged	authored
`/after_pull_request`	Wait for the given pull request id to be merged before continuing with the current one.
`/bypass_author_approval`	Bypass the pull request author's approval	⭐
`/bypass_build_status`	Bypass the build and test status	⭐
`/bypass_commit_size`	Bypass the check on the size of the changeset `TBA`	⭐
`/bypass_incompatible_branch`	Bypass the check on the source branch prefix	⭐
`/bypass_jira_check`	Bypass the Jira issue check	⭐
`/bypass_peer_approval`	Bypass the pull request peers' approval	⭐
`/bypass_leader_approval`	Bypass the pull request leaders' approval	⭐
`/approve`	Instruct Bert-E that the author has approved the pull request.		✍️
`/create_pull_requests`	Allow the creation of integration pull requests.
`/create_integration_branches`	Allow the creation of integration branches.
`/no_octopus`	Prevent Wall-E from doing any octopus merge and use multiple consecutive merge instead
`/unanimity`	Change review acceptance criteria from `one reviewer at least` to `all reviewers`
`/wait`	Instruct Bert-E not to run until further notice.

Available commands

name	description	privileged
`/help`	Print Bert-E's manual in the pull request.
`/status`	Print Bert-E's current status in the pull request `TBA`
`/clear`	Remove all comments from Bert-E from the history `TBA`
`/retry`	Re-start a fresh build `TBA`
`/build`	Re-start a fresh build `TBA`
`/force_reset`	Delete integration branches & pull requests, and restart merge process from the beginning.
`/reset`	Try to remove integration branches unless there are commits on them which do not appear on the source branch.

Status report is not available.

bert-e · 2024-11-06T18:28:50Z

Incorrect fix version

The Fix Version/s in issue BB-614 contains:

None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

8.7.0

Please check the Fix Version/s of BB-614, or the target
branch of this pull request.

Issue: BB-614

codecov · 2024-11-12T15:57:01Z

Codecov Report

Attention: Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.

Project coverage is 71.19%. Comparing base (6694ca8) to head (64450ba).
Report is 36 commits behind head on development/8.7.

Files with missing lines	Patch %	Lines
lib/queuePopulator/BucketFileLogReader.js	0.00%	1 Missing ⚠️
lib/queuePopulator/RaftLogReader.js	0.00%	1 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
extensions/mongoProcessor/MongoQueueProcessor.js	`76.47% <100.00%> (+13.75%)`	⬆️
...sions/replication/queueProcessor/QueueProcessor.js	`73.11% <ø> (+0.14%)`	⬆️
...ationStatusProcessor/ReplicationStatusProcessor.js	`67.76% <ø> (-0.56%)`	⬇️
extensions/replication/tasks/CopyLocationTask.js	`63.38% <ø> (-0.54%)`	⬇️
...xtensions/replication/tasks/MultipleBackendTask.js	`53.57% <ø> (+0.28%)`	⬆️
extensions/replication/tasks/ReplicateObject.js	`91.36% <100.00%> (-0.13%)`	⬇️
...sions/replication/tasks/UpdateReplicationStatus.js	`77.19% <ø> (-1.31%)`	⬇️
lib/queuePopulator/IngestionPopulator.js	`69.56% <ø> (+2.67%)`	⬆️
lib/queuePopulator/IngestionReader.js	`80.88% <100.00%> (-0.73%)`	⬇️
lib/queuePopulator/KafkaLogReader.js	`100.00% <100.00%> (+81.25%)`	⬆️
... and 5 more

... and 4 files with indirect coverage changes

Components	Coverage Δ
Bucket Notification	`66.66% <ø> (ø)`
Core Library	`77.15% <60.00%> (+2.01%)`	⬆️
Ingestion	`72.09% <100.00%> (+2.85%)`	⬆️
Lifecycle	`76.20% <ø> (ø)`
Oplog Populator	`82.95% <ø> (ø)`
Replication	`57.15% <100.00%> (-0.64%)`	⬇️
Bucket Scanner	`85.60% <ø> (ø)`

@@                 Coverage Diff                 @@
##           development/8.7    #2588      +/-   ##
===================================================
+ Coverage            70.31%   71.19%   +0.88%     
===================================================
  Files                  194      192       -2     
  Lines                12997    12722     -275     
===================================================
- Hits                  9139     9058      -81     
+ Misses                3848     3654     -194     
  Partials                10       10

Flag	Coverage Δ
api:retry	`9.94% <0.00%> (+0.21%)`	⬆️
api:routes	`9.74% <0.00%> (+0.21%)`	⬆️
bucket-scanner	`85.60% <ø> (ø)`
ingestion	`12.17% <7.14%> (-0.29%)`	⬇️
lib	`7.67% <0.00%> (+0.16%)`	⬆️
lifecycle	`19.66% <0.00%> (+0.42%)`	⬆️
notification	`0.89% <0.00%> (+0.01%)`	⬆️
replication	`18.46% <7.14%> (-0.57%)`	⬇️
unit	`47.98% <85.71%> (+1.37%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

williamlardier

the changes as such lgtm, but I am not sure if we have other places to handle, I'll let other reviewers confirm - I'm trusting here the green build

I just see in the tests/functional/replication/streamedCopy.spec.js test file some previous mock still there

williamlardier · 2024-11-18T14:49:50Z

extensions/mongoProcessor/MongoQueueProcessor.js

@@ -97,32 +77,15 @@ class MongoQueueProcessor {
     * @return {undefined}
     */
    start() {
-        this.logger.info('starting mongo queue processor');


We can keep this log, won't hurt

williamlardier · 2024-11-18T14:52:09Z

extensions/mongoProcessor/MongoQueueProcessor.js

+                this.logger.error('could not connect to MongoDB', {
+                    method: 'MongoQueueProcessor.start',
+                    error: err.message,
+                });
                this.logger.fatal('error starting mongo queue processor');


No need to log twice I would say, just log the fatal message with all the info

Suggested change

this.logger.error('could not connect to MongoDB', {

method: 'MongoQueueProcessor.start',

error: err.message,

});

this.logger.fatal('error starting mongo queue processor');

this.logger.fatal('could not connect to MongoDB, error starting mongo queue processor', {

method: 'MongoQueueProcessor.start',

error: err.message,

});

Kerkesni · 2024-11-18T16:11:09Z

extensions/replication/replicationStatusProcessor/ReplicationStatusProcessor.js

@@ -336,7 +334,6 @@ class ReplicationStatusProcessor {
            sourceHTTPAgent: this.sourceHTTPAgent,
            vaultclientCache: this.vaultclientCache,
            gcProducer: this._gcProducer,
-            mProducer: this._mProducer,
            statsClient: this._statsClient,


I think _statsClient is also part of the "Redis metrics". To be double checked, but i think it also needs to be removed.

Same in other files.

Kerkesni · 2024-11-18T17:06:09Z

lib/queuePopulator/IngestionPopulator.js

-        // metrics clients
-        this._mProducer = null;
-        this._mConsumer = null;
-        this._redis = null;


Should keep this line

bert-e · 2024-11-20T10:40:12Z

Incorrect fix version

The Fix Version/s in issue BB-614 contains:

None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

9.0.0

Please check the Fix Version/s of BB-614, or the target
branch of this pull request.

francoisferrand

In cloudserver's reportHandler, there are queries on CRR metrics : will this not break if we remove these redis metrics?

bert-e · 2024-11-26T09:24:23Z

Branches have diverged

This pull request's source branch improvement/BB-614-remove-redis-metrics has diverged from
development/9.0 by more than 50 commits.

To avoid any integration risks, please re-synchronize them using one of the
following solutions:

Merge origin/development/9.0 into improvement/BB-614-remove-redis-metrics
Rebase improvement/BB-614-remove-redis-metrics onto origin/development/9.0

Note: If you choose to rebase, you may have to ask me to rebuild
integration branches using the reset command.

benzekrimaha force-pushed the improvement/BB-614-remove-redis-metrics branch 2 times, most recently from d88e505 to 78f5513 Compare November 7, 2024 14:56

benzekrimaha added 3 commits November 7, 2024 15:57

remove metricsConsumer and Producer

8349ad7

Issue: BB-614

adapt log after producer and consumer removal

a666f19

Issue: BB-614

adapt mongoQueueProcessor func test

2bfe58e

Issue: BB-614

benzekrimaha force-pushed the improvement/BB-614-remove-redis-metrics branch from 78f5513 to 2bfe58e Compare November 7, 2024 14:57

scality deleted a comment from codecov bot Nov 7, 2024

benzekrimaha force-pushed the improvement/BB-614-remove-redis-metrics branch 2 times, most recently from 69d5cf3 to 7c68d96 Compare November 12, 2024 13:21

tests add for coverage

64450ba

Issue: BB-614

benzekrimaha force-pushed the improvement/BB-614-remove-redis-metrics branch from 7c68d96 to 64450ba Compare November 12, 2024 15:56

scality deleted a comment from codecov bot Nov 12, 2024

benzekrimaha marked this pull request as ready for review November 12, 2024 15:56

benzekrimaha requested review from williamlardier, francoisferrand, Kerkesni and KillianG and removed request for williamlardier and francoisferrand November 13, 2024 17:25

williamlardier approved these changes Nov 18, 2024

View reviewed changes

Kerkesni reviewed Nov 18, 2024

View reviewed changes

francoisferrand changed the base branch from development/8.7 to development/9.0 November 20, 2024 10:40

francoisferrand reviewed Nov 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvement/bb 614 remove redis metrics #2588

Improvement/bb 614 remove redis metrics #2588

benzekrimaha commented Nov 6, 2024 •

edited

Loading

bert-e commented Nov 6, 2024

bert-e commented Nov 6, 2024

codecov bot commented Nov 12, 2024 •

edited

Loading

williamlardier left a comment

williamlardier Nov 18, 2024

williamlardier Nov 18, 2024

Kerkesni Nov 18, 2024

Kerkesni Nov 18, 2024

Kerkesni Nov 18, 2024

bert-e commented Nov 20, 2024

francoisferrand left a comment

bert-e commented Nov 26, 2024

Improvement/bb 614 remove redis metrics #2588

Are you sure you want to change the base?

Improvement/bb 614 remove redis metrics #2588

Conversation

benzekrimaha commented Nov 6, 2024 • edited Loading

bert-e commented Nov 6, 2024

Hello benzekrimaha,

bert-e commented Nov 6, 2024

Incorrect fix version

codecov bot commented Nov 12, 2024 • edited Loading

Codecov Report

williamlardier left a comment

Choose a reason for hiding this comment

williamlardier Nov 18, 2024

Choose a reason for hiding this comment

williamlardier Nov 18, 2024

Choose a reason for hiding this comment

Kerkesni Nov 18, 2024

Choose a reason for hiding this comment

Kerkesni Nov 18, 2024

Choose a reason for hiding this comment

Kerkesni Nov 18, 2024

Choose a reason for hiding this comment

bert-e commented Nov 20, 2024

Incorrect fix version

francoisferrand left a comment

Choose a reason for hiding this comment

bert-e commented Nov 26, 2024

Branches have diverged

benzekrimaha commented Nov 6, 2024 •

edited

Loading

codecov bot commented Nov 12, 2024 •

edited

Loading