BB-721: Properly get location in lifecycle metrics by benzekrimaha · Pull Request #2736 · scality/backbeat

benzekrimaha · 2026-04-30T08:19:08Z

codecov · 2026-05-21T15:49:14Z

Codecov Report

❌ Patch coverage is 80.37383% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.62%. Comparing base (5d84665) to head (326294c).

Files with missing lines	Patch %	Lines
extensions/lifecycle/tasks/LifecycleTask.js	77.38%	19 Missing ⚠️
extensions/lifecycle/util/lifecycleLocation.js	90.47%	2 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
...sions/lifecycle/tasks/LifecycleDeleteObjectTask.js	`92.94% <100.00%> (+0.04%)`	⬆️
extensions/lifecycle/util/lifecycleLocation.js	`90.47% <90.47%> (ø)`
extensions/lifecycle/tasks/LifecycleTask.js	`89.93% <77.38%> (-1.69%)`	⬇️

... and 3 files with indirect coverage changes

Components	Coverage Δ
Bucket Notification	`80.37% <ø> (ø)`
Core Library	`80.50% <ø> (-0.61%)`	⬇️
Ingestion	`71.14% <ø> (ø)`
Lifecycle	`79.36% <80.37%> (+0.01%)`	⬆️
Oplog Populator	`85.83% <ø> (ø)`
Replication	`59.61% <ø> (ø)`
Bucket Scanner	`85.76% <ø> (ø)`

@@                                Coverage Diff                                 @@
##           improvement/BB-740-monitor-lifecycle-conductor    #2736      +/-   ##
==================================================================================
- Coverage                                           74.83%   74.62%   -0.22%     
==================================================================================
  Files                                                 200      201       +1     
  Lines                                               13701    13796      +95     
==================================================================================
+ Hits                                                10253    10295      +42     
- Misses                                               3438     3491      +53     
  Partials                                               10       10

Flag	Coverage Δ
api:retry	`9.02% <0.00%> (-0.07%)`	⬇️
api:routes	`8.84% <0.00%> (-0.07%)`	⬇️
bucket-scanner	`85.76% <ø> (ø)`
ft_test:queuepopulator	`9.01% <0.00%> (-1.88%)`	⬇️
ingestion	`12.38% <0.00%> (-0.09%)`	⬇️
lib	`7.52% <0.00%> (-0.05%)`	⬇️
notification	`1.01% <0.00%> (-0.01%)`	⬇️
oplogPopulator	`0.14% <0.00%> (-0.01%)`	⬇️
replication	`18.28% <0.00%> (-0.13%)`	⬇️
unit	`51.93% <80.37%> (+0.44%)`	⬆️

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Use archive-aware HeadObject, object metadata, and bucket location fallback to avoid reporting STANDARD when a real lifecycle location is available. Issue: BB-721

Resolve delete and restored-object expiration metrics from object metadata so restored objects report their cold location. Issue: BB-721

SylvainSenechal · 2026-05-22T09:00:09Z

+    return objectMD[fieldName];
+}
+
+function isRealLocation(location) {


Still reading the pr, but I dont fully understand the link between the function name and what its doing.

Basically checking that a given location is non null and not equal to 'STANDARD'. So why not name it "isStandardLocation" or "isNotStandardLocation" ?

Reading a bit further, I think the standard location is some kind of default value, and we try to resolve some locations, so maybe the intent is "isResolvedLocations" ? I think some of this can be documented by code comments too

update : looking at the jira ticket, it gives more context : "In some cases, we have STANDARD as location in lifecycle metrics : as we get object information through regular S3 API."

I think this is the kind of context that could be added as code comment

SylvainSenechal · 2026-05-22T09:08:40Z

+
+const STANDARD_LOCATION = 'STANDARD';
+
+function _getObjectMDValue(objectMD, getterName, fieldName) {


Haven't digged too much into this, but I'm surprised we have to create a helper function for this, since there must already be some existing code trying to access object metadatas, I wonder how it's being done ?

Don't we have getters functions in Arsenal for this already ?

Looking a bit more into it : The function is only called locally from resolveLifecycleMetricObjectLocation, we can check that objectMD is non nil directly at the beginning of resolveLifecycleMetricObjectLocation,
then when it's not nil, I think we don't need this helper and can directly call
objectMD.getArchive()
objectMD..getDatastoreName()
etc

I agree with Sylvain on this one, something is not clear

I think the idea is to be able to manage when the value are wrapped in a class (Arsenal) or when it's a raw data. But we should try to always manipulate class ?

SylvainSenechal · 2026-05-22T09:14:43Z

+    return !isRealLocation(location);
+}
+
+function resolveLifecycleMetricObjectLocation(objectMD, fallbackLocation) {


I find it a bit weird that in this function, we are calling isRealLocation on different object metadata attributes. Maybe its fine but it probably deserve some code comments to explain a bit more what its doing

Can also explain why we only look at the first locations[0], of course future developers can always ask an llm to know (claude said its because mpu object have an array of locations, but all locations are the same so its ok to look at the first only) but I think its nicer to have a human written comment for things that can be confusing. Maybe here it does deserves a helper function like "getMpuObjectLocation" or more generic "getObjectLocation" that handle both mpu and non mpu objects

SylvainSenechal · 2026-05-22T09:20:10Z

Can discuss with other reviewers : its a bit annoying to do one more pr but, should all these functions belongs to Arsenal ?

SylvainSenechal · 2026-05-22T09:34:19Z

+    return !!location && location !== STANDARD_LOCATION;
+}
+
+function shouldResolveLifecycleMetricLocation(location) {


after reading the pr some more, I think this is more a
needsResolveLifecycleMetricLocation
than
shouldResolveLifecycleMetricLocation

It makes the function calling it easier to understand what they are doing

I'm not sure we need a new function to just call an another one ?

SylvainSenechal · 2026-05-22T09:53:42Z

+        }, false);
+    }
+
+    _resolveLifecycleMetricLocation(entry, fallbackLocation, log, cb) {


i feel like something is off around here :
We have
_resolveLifecycleMetricLocation calling

_resolveLifecycleMetricLocationFromBucket

_resolveLifecycleMetricLocationFromMetadata which calls these 2

_resolveLifecycleMetricLocationFromArchiveInfo

_resolveLifecycleMetricLocationFromBucket

So Frombucket can be called twice, and we have something else smelly : the if entry action type == deleteMpu is checked in this function, but also in _resolveLifecycleMetricLocationFromMetadata

I have a feeling it would be better to have a single function, even if larger, that can be read more naturally with less indirection.

Discussed this with Claude and providing his answer here :

DarkIsDude

Can you also tell me more about In some cases, we have STANDARD as location in lifecycle metrics : as we get object information through regular S3 API. which cases and when it's an issue to use regular s3 api ?

DarkIsDude · 2026-05-22T09:53:34Z

+
+const STANDARD_LOCATION = 'STANDARD';
+
+function _getObjectMDValue(objectMD, getterName, fieldName) {


I agree with Sylvain on this one, something is not clear

DarkIsDude · 2026-05-22T09:55:08Z

+    return !!location && location !== STANDARD_LOCATION;
+}
+
+function shouldResolveLifecycleMetricLocation(location) {


I'm not sure we need a new function to just call an another one ?

DarkIsDude · 2026-05-22T09:57:20Z

+
+const STANDARD_LOCATION = 'STANDARD';
+
+function _getObjectMDValue(objectMD, getterName, fieldName) {


I think the idea is to be able to manage when the value are wrapped in a class (Arsenal) or when it's a raw data. But we should try to always manipulate class ?

francoisferrand

no clear which scenario exactly creates which problem - and hence if the fix is relevant
by trying to be very generic, the code creates lots of complexity, with a very complex algorithm being implemented to retrieve the location in different cases - which also makes it very difficult to be sure "what" will actually end up in the metrics
this is a "small" improvement: instead of STANDARD we get in some case, we want to get the actual field which is already in metadata; so don't want to pay for this (if it needs so much complexity and IOs, may be better to just live with it)

With the advances in Cloudserver api (headObject, list object v2, ...), is there no way to just tweak the existing approach, to get to this information efficiently and keep this simple?

francoisferrand · 2026-05-26T13:14:36Z

-        this.producer.sendToTopic(this.objectTasksTopic, entries,  err => {
-            LifecycleMetrics.onKafkaPublish(null, 'ObjectTopic', 'bucket', err, 1);
-            return cb(err);
+        const command = new GetBucketLocationCommand({ Bucket: bucket });


getBucketLocation() returns the default location of a bucket, which could change (it happens in S3C, in some cases):

so would be a race condition here...

the cache makes this issue worse, as it may get poisoned by the "old" value of the location

francoisferrand · 2026-05-26T13:15:15Z

+            .then(data => {
+                LifecycleMetrics.onS3Request(log, 'getBucketLocation', 'bucket', null);
+                const location = data && data.LocationConstraint;
+                this._bucketLocationCache.set(bucket, location);


there may be many buckets, not sure we should allow unbounded cache

francoisferrand · 2026-05-26T13:19:01Z

-        if (shouldBreak) {
-            process.nextTick(() => cb(errorCircuitBreakerTripped));
-            return;
+        this._resolveLifecycleMetricLocation(entry, initialLocation, this.log, (err, location) => {


instead of the extra metadata call, could we change one (existing) call?
i.e. if we already call HeadObject, we could instead pass the appropriate flag (x-amz-scal-archive-info) to get more details? Or use getObjectAttribute/listObjectV2 with attributes to get the required info more efficiently?

francoisferrand · 2026-05-26T13:20:46Z

     */
    _sendObjectAction(entry, cb) {
-        const location = entry.getAttribute('details.dataStoreName');
+        const initialLocation = entry.getAttribute('details.dataStoreName');


this is not the "initialLocation" : if the location is already set (e.g. object has been transitioned already or written directly to a non-default location), this value should be correct already : and it must be used.
→ this is really the "real" location, except sometimes it is "STANDARD" (i.e. default bucket location) and needs to be resolved

also sometimes needs to be resolved because it is a "cold" location, according to the ticket.

Though practically, not sure what the problem really is - of if there one : sendObjectAction would be called on expiration, and seems ok if we report the expiration of a cold object as happening on the "cold" location itself?

francoisferrand · 2026-05-26T13:24:32Z

+            });
+    }
+
+    _resolveLifecycleMetricLocationFromBucket(entry, fallbackLocation, log, cb) {


this so-called "fallbackLocation" is NOT a fallback, this is the actual location

parameter is redundant : this is part of entry already

francoisferrand · 2026-05-26T13:34:51Z

    }

+    _resolveLifecycleMetricLocationFromMetadata(entry, fallbackLocation, log, cb) {
+        if (!this.backbeatMetadataProxy) {


why do we need 3 cases?

backbeatMetadataProxy is always available AFAICS (always initialized in Bucket Processor)

resolving from ObjectMD or Bucket should work in all cases : but at the cost of extra IO (read ObjectMD) or race condition (read default bucket location)

francoisferrand · 2026-05-26T13:39:26Z

+                    versionId: params.versionId,
+                    error: err.message,
+                });
+                return this._resolveLifecycleMetricLocationFromArchiveInfo(entry, fallbackLocation, log, cb);


why make the call to getObjectMD if we already had the data in entry ?

francoisferrand · 2026-05-26T13:40:04Z

+
+const STANDARD_LOCATION = 'STANDARD';
+
+function _getObjectMDValue(objectMD, getterName, fieldName) {


not possible. When you call a function, you MUST know what the parameters are : and then you implement the way which is needed, instead of trying to cope with various cases dynamically...

francoisferrand · 2026-05-26T13:55:16Z

        });
    });

+    it('should emit expiration metrics with restored object cold location', done => {


expiration metrics should be made with cold location: since this is where the data is "really" stored...
Or are there some cases where this creates inconsistencies?

francoisferrand · 2026-05-26T14:11:14Z

            "Effect": "Allow",
            "Action": [
                "s3:GetLifecycleConfiguration",
+                "s3:GetBucketLocation",


also need extra permission to get "archive info" in HeadObject

benzekrimaha force-pushed the improvement/BB-740-monitor-lifecycle-conductor branch 3 times, most recently from 9da1c41 to 5561284 Compare May 4, 2026 09:33

benzekrimaha force-pushed the improvement/BB-740-monitor-lifecycle-conductor branch 5 times, most recently from 4da31b3 to 4520f5a Compare May 21, 2026 14:38

benzekrimaha force-pushed the improvement/BB-721-properly-get-location-in-lifecycle-metrics branch from f9d1347 to 7c00e43 Compare May 21, 2026 15:46

scality deleted a comment from codecov Bot May 21, 2026

benzekrimaha marked this pull request as ready for review May 21, 2026 15:48

benzekrimaha changed the title ~~Improvement/bb 721 properly get location in lifecycle metrics~~ BB-721: Properly get location in lifecycle metrics May 21, 2026

benzekrimaha force-pushed the improvement/BB-721-properly-get-location-in-lifecycle-metrics branch from 7c00e43 to 405b79a Compare May 21, 2026 16:04

benzekrimaha requested review from a team, DarkIsDude and SylvainSenechal May 21, 2026 16:04

benzekrimaha added 2 commits May 21, 2026 18:07

Resolve lifecycle metric locations before queuing actions.

54f6a60

Use archive-aware HeadObject, object metadata, and bucket location fallback to avoid reporting STANDARD when a real lifecycle location is available. Issue: BB-721

Use real lifecycle locations for object task metrics.

326294c

Resolve delete and restored-object expiration metrics from object metadata so restored objects report their cold location. Issue: BB-721

benzekrimaha force-pushed the improvement/BB-721-properly-get-location-in-lifecycle-metrics branch from 405b79a to 326294c Compare May 21, 2026 16:07

SylvainSenechal reviewed May 22, 2026

View reviewed changes

DarkIsDude reviewed May 22, 2026

View reviewed changes

francoisferrand reviewed May 26, 2026

View reviewed changes


		const STANDARD_LOCATION = 'STANDARD';

		function _getObjectMDValue(objectMD, getterName, fieldName) {

Conversation

benzekrimaha commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SylvainSenechal May 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SylvainSenechal May 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DarkIsDude left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

francoisferrand left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

benzekrimaha commented Apr 30, 2026 •

edited

Loading

codecov Bot commented May 21, 2026 •

edited

Loading

SylvainSenechal May 22, 2026 •

edited

Loading

SylvainSenechal May 22, 2026 •

edited

Loading