Properly get location in lifecycle metrics#2736
Conversation
benzekrimaha
force-pushed
the
improvement/BB-740-monitor-lifecycle-conductor
branch
3 times, most recently
from
9da1c41 to
5561284
Compare
benzekrimaha
force-pushed
the
improvement/BB-740-monitor-lifecycle-conductor
branch
5 times, most recently
from
4da31b3 to
4520f5a
Compare
benzekrimaha
force-pushed
the
improvement/BB-721-properly-get-location-in-lifecycle-metrics
branch
from
f9d1347 to
7c00e43
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
... and 2 files with indirect coverage changes
@@ Coverage Diff @@
## development/9.5 #2736 +/- ##
===================================================
- Coverage 75.16% 75.11% -0.05%
===================================================
Files 200 200
Lines 13839 13844 +5
===================================================
- Hits 10402 10399 -3
- Misses 3427 3435 +8
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
benzekrimaha
changed the title
benzekrimaha
force-pushed
the
improvement/BB-721-properly-get-location-in-lifecycle-metrics
branch
from
7c00e43 to
405b79a
Compare
benzekrimaha
force-pushed
the
improvement/BB-721-properly-get-location-in-lifecycle-metrics
branch
from
405b79a to
326294c
Compare
| return objectMD[fieldName]; | ||
| } | ||
|
|
||
| function isRealLocation(location) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I removed the ambiguous resolution helpers from the bucket task path. The object action now uses the location already set on the entry.
There was a problem hiding this comment.
Done differently: the STANDARD resolution is no longer hidden in this helper. For non-versioned objects, the existing HeadObject call with archive info sets the real StorageClass before the action is queued.
|
|
||
| const STANDARD_LOCATION = 'STANDARD'; | ||
|
|
||
| function _getObjectMDValue(objectMD, getterName, fieldName) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I agree with Sylvain on this one, something is not clear
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
Removed the generic helper usage from LifecycleTask; no more mixed raw/ObjectMD handling in that path. The resolution is now explicit at the existing HeadObject call site. I also removed the dynamic raw/class abstraction from the bucket task path.
| return !isRealLocation(location); | ||
| } | ||
|
|
||
| function resolveLifecycleMetricObjectLocation(objectMD, fallbackLocation) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Removed this logic from the bucket task path, so the extra comment is no longer needed there.We no longer inspect ObjectMD/location arrays while queuing object actions
There was a problem hiding this comment.
Can discuss with other reviewers : its a bit annoying to do one more pr but, should all these functions belongs to Arsenal ?
There was a problem hiding this comment.
Not needed anymore for this PR scope: most of the helper logic was removed. The remaining object-processor helper only handles an ObjectMD already fetched for deletion.
| return !!location && location !== STANDARD_LOCATION; | ||
| } | ||
|
|
||
| function shouldResolveLifecycleMetricLocation(location) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I'm not sure we need a new function to just call an another one ?
There was a problem hiding this comment.
The function was removed
| }, false); | ||
| } | ||
|
|
||
| _resolveLifecycleMetricLocation(entry, fallbackLocation, log, cb) { |
There was a problem hiding this comment.
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 :
There was a problem hiding this comment.
Removed the multi-step resolution chain. _sendObjectAction no longer performs metadata/head/bucket lookups
DarkIsDude
left a comment
DarkIsDude
left a comment
There was a problem hiding this comment.
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 ?
|
|
||
| const STANDARD_LOCATION = 'STANDARD'; | ||
|
|
||
| function _getObjectMDValue(objectMD, getterName, fieldName) { |
There was a problem hiding this comment.
I agree with Sylvain on this one, something is not clear
| return !!location && location !== STANDARD_LOCATION; | ||
| } | ||
|
|
||
| function shouldResolveLifecycleMetricLocation(location) { |
There was a problem hiding this comment.
I'm not sure we need a new function to just call an another one ?
|
|
||
| const STANDARD_LOCATION = 'STANDARD'; | ||
|
|
||
| function _getObjectMDValue(objectMD, getterName, fieldName) { |
There was a problem hiding this comment.
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
left a comment
francoisferrand
left a comment
There was a problem hiding this comment.
- 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?
benzekrimaha
force-pushed
the
improvement/BB-740-monitor-lifecycle-conductor
branch
from
e02d223 to
e7ce2c6
Compare
benzekrimaha
force-pushed
the
improvement/BB-721-properly-get-location-in-lifecycle-metrics
branch
from
9631932 to
d8ce536
Compare
benzekrimaha
changed the title
francoisferrand
left a comment
francoisferrand
left a comment
There was a problem hiding this comment.
very difficult to review this PR, missing the analysis of the ticket : what problems exactly is it solving? in which situation do we have an issue, and why?
→ please document this in the ticket and PR
| { LastModified: data.LastModified }); | ||
| { | ||
| LastModified: data.LastModified, | ||
| StorageClass: data.StorageClass || obj.StorageClass, |
There was a problem hiding this comment.
why the "or" ?
- storageClass will always be set
- it will the "cold" class (object.x-amz-storage-class) for cold object, even if restored
- it will never be STANDARD (contrary to obj.StorageClass), i.e. always the "real" storage even if not default
There was a problem hiding this comment.
Indeed. With the x-amz-scal-archive-info header, CloudServer always sets x-amz-storage-class in the response (setArchiveInfoHeaders returns objMD['x-amz-storage-class'] || objMD.dataStoreName), so data.StorageClass is always the real class, the preserved cold class for cold/restored objects, the data-store name otherwise , and never STANDARD. The fallback was dead code and reintroduced the listing ambiguity we aiming to remove: dropped it, and I documented the contract in a comment at the call site.
| const location = _getLifecycleMetricLocation( | ||
| this.objectMD, | ||
| entry.getAttribute('details.dataStoreName') |
There was a problem hiding this comment.
with the headObject change, the storage class is "initially" correct.
would it be enough to just trust entry.getAttribute('details.dataStoreName') now ?
benzekrimaha
force-pushed
the
improvement/BB-740-monitor-lifecycle-conductor
branch
from
e7ce2c6 to
f5bb822
Compare
DarkIsDude
left a comment
DarkIsDude
left a comment
There was a problem hiding this comment.
LGTM but I'm still newbie to backbeat so I suggest to have at least one review from someone who worked on it 🚀
benzekrimaha
force-pushed
the
improvement/BB-740-monitor-lifecycle-conductor
branch
from
321a95b to
bc45681
Compare
|
francoisferrand
left a comment
francoisferrand
left a comment
There was a problem hiding this comment.
very difficult to review this PR, missing the analysis of the ticket : what problems exactly is it solving? in which situation do we have an issue, and why?
→ please document this in the ticket and PR
benzekrimaha
force-pushed
the
improvement/BB-721-properly-get-location-in-lifecycle-metrics
branch
from
d8ce536 to
813da92
Compare
|
@francoisferrand I've updated the PR description with a full problem analysis (root cause + the two concrete scenarios where the location label is wrong: cold-storage expiration and restored-object cleanup). |
|
Added problem analysis to the PR description and BB-721 ticket per review feedback. TL;DR for reviewers: Expiration metrics can report the wrong location because lifecycle used listing/STANDARD semantics and, for restored cold objects, completion metrics preferred warm objectMD.dataStoreName over the cold location on the action entry. The fix resolves the real storage class at queue time via archive-info HeadObject, then uses details.dataStoreName downstream. |
| { LastModified: data.LastModified }); | ||
| { | ||
| LastModified: data.LastModified, | ||
| StorageClass: data.StorageClass || obj.StorageClass, |
| }); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
I wonder if these unit tests are enough: the issue was a correlation between different values at different layers (STANDARD or cold class in ListObject/HeadObject, hot class in GetMetadata, the way the kafka message's dataStoreName was computed...):
- should we have some sort higher level test where we exercise or simulate the whole sequence?
- do the existing tests cover every kind of lifecycle op (expiration of hot / cold / restored objects, transition from hot to cold, transition from STANDARD to hot, transition from hot to STANDARD...) and verify the metrics are appropriate eventually?
There was a problem hiding this comment.
I extended the coverage into a scenario matrix at the queue level: expiration of a hot object on the default location (listing says STANDARD, HeadObject returns the real location), a hot object on a non-default location, a cold object, and a restored cold object (cold class preserved in x-amz-storage-class). Each case asserts the archive-info middleware is attached to the HeadObject and that the queued entry's details.dataStoreName is the head-resolved storage class. The execution side (metric emitted from the entry location) is covered in LifecycleDeleteObjectTask.spec.js and LifecycleUpdateExpirationTask.spec.js, including the restored-object case.
Transitions are not affected by this change: transition trigger metrics use the rule's target site, not the listed storage class.
A full end-to-end test (listing → HeadObject → Kafka entry → processor → metric) would need the functional suite with a real CloudServer; I'd propose tracking that as a follow-up ticket rather than blocking this fix WDYT?
benzekrimaha
force-pushed
the
improvement/BB-740-monitor-lifecycle-conductor
branch
from
e24669e to
df55485
Compare
Hello benzekrimaha,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Allow the lifecycle bucket processor to request archive info during HeadObject and cover the policy so real metric location resolution keeps working. Issue: BB-721
Request archive info on the existing HeadObject call and reuse the returned storage class when queuing lifecycle expiration actions, so trigger metrics keep the real object location without extra metadata or bucket-location lookups. Issue: BB-721
_compareObject issues a HeadObject with archive info and stores the resolved storage class (the preserved cold class for cold and restored objects) in the action entry's details.dataStoreName. The expiration metric therefore reports the location the action was queued against: drop the objectMD.dataStoreName override, which reported the warm location for restored cold objects, and cover the restored-object case in the delete and update-expiration task specs. Issue: BB-721
benzekrimaha
force-pushed
the
improvement/BB-721-properly-get-location-in-lifecycle-metrics
branch
from
813da92 to
764f982
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Set a warm dataStoreName on the mocked object metadata so the test reflects the restored-cold-object scenario: metadata carries the warm location while the entry carries the cold one, and the metric must use the entry location. Issue: BB-721
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following reviewers are expecting changes from the author, or must review again: |
5 participants
Issue: BB-721
Context analysis
Lifecycle execution metrics (
s3_lifecycle_started_total/s3_lifecycle_completed_total, emitted viaLifecycleMetrics.onLifecycleStarted/onLifecycleCompleted) carry alocationlabel used by Grafana panels and alerts to break lifecycle activity down by storage backend.For expiration of a restored cold object, that label was wrong: the completion metric reported the object's warm location instead of the cold location the archive actually lives on. On platforms with cold storage this made per-location panels and alerts unusable for expiration of restored data.
Root cause
LifecycleDeleteObjectTaskcomputed the metric location as:For a restored cold object, CloudServer's restore-completion path (
overwritingVersioningincloudserver:lib/api/apiUtils/object/versioning.js) deliberately:x-amz-storage-classas the cold location name (e.g.location-dmf-v1), anddataStoreNameto the warm location where the restored data was actually written.So
objectMD.dataStoreNameis the warm location, while the queued action entry'sdetails.dataStoreName(set at queue time inLifecycleTaskfrom the listing'sStorageClass, which mirrorsx-amz-storage-class) is the cold location. TheobjectMD || entryprecedence therefore picked the wrong one and mislabelled the metric.LifecycleUpdateExpirationTaskwas not affected — it already resolves the metric location fromtarget.location(the cold location).Fix implemented in this PR
LifecycleDeleteObjectTask: drop theobjectMD.dataStoreNameoverride and useentry.getAttribute('details.dataStoreName')directly, so the completion metric reports the same location the action was queued against.LifecycleTask._compareObject: make the queueddetails.dataStoreNamean explicit, authoritative source instead of reusingobj.StorageClassfrom the listing verbatim. On theHeadObjectcall already issued per candidate, attachx-amz-scal-archive-info: true(via SDK middleware) so the response always carries an explicit storage class — including the plainSTANDARDcase, which CloudServer's defaultHeadObjectpath strips (collectResponseHeaders) butsetArchiveInfoHeadersalways sets. Propagate the resultingStorageClassonto the candidate object asdata.StorageClass || obj.StorageClass, so downstream queuing sees a consistent, non-empty value regardless of whether the object is on the default location, a warm non-default location, cold, or restored-from-cold.extensions/lifecycle/bucketProcessor/policy.json: grantscality:GetObjectArchiveInfoso the bucket processor is allowed to send the archive-info request header.Propagate the resulting StorageClass onto the candidate object (data.StorageClass — always set by CloudServer when the archive-info header is present, never STANDARD)
Tests
New / updated unit tests:
LifecycleTask.spec.js:HeadObjectis sent withx-amz-scal-archive-info: true, the returnedStorageClassis propagated onto the candidate, and the queued entry'sdetails.dataStoreNamereflects it (including for restored / cold objects).LifecycleDeleteObjectTask.spec.js: expiration completion metric uses the queueddetails.dataStoreName, no longer influenced byobjectMD.dataStoreName; covers the restored-cold-object case.LifecycleUpdateExpirationTask.spec.js: regression assertion thatonLifecycleStarted/onLifecycleCompletedare called with the cold location for restored-object expiration.LifecycleBucketProcessorPolicy.spec.js: policy exposes the newscality:GetObjectArchiveInfoaction.=> the _compareObject test is now a four-scenario matrix (hot default / hot non-default / cold / restored)