-
Notifications
You must be signed in to change notification settings - Fork 249
CLDSRV-746: Adding log fields to the backbeat routes in order to fill the analytics dashboard #5955
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CLDSRV-746: Adding log fields to the backbeat routes in order to fill the analytics dashboard #5955
Conversation
Hello fredmnl,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
Codecov Report❌ Patch coverage is
Additional details and impacted files
@@ Coverage Diff @@
## development/9.0 #5955 +/- ##
================================================
Coverage 83.28% 83.28%
================================================
Files 189 189
Lines 12128 12160 +32
================================================
+ Hits 10101 10128 +27
- Misses 2027 2032 +5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command:
Alternatively, the |
lib/routes/routeBackbeat.js
Outdated
const actionMap = { | ||
'expiration': 'BackbeatExpiration', | ||
'batchdelete': 'BackbeatBatchDelete', | ||
'data': 'BackbeatPutData', | ||
'metadata': 'BackbeatPutMetadata', | ||
'multiplebackenddata': 'BackbeatMultipleBackendData', | ||
'multiplebackendmetadata': 'BackbeatMultipleBackendMetadata', | ||
'index': 'BackbeatIndexOperation', | ||
'api': 'BackbeatApi', | ||
'lifecycle': 'BackbeatListLifecycle', | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Declare this mapping out of the routing to assign it only once and not per request.
For the value I'd remove the Backbeat
prefix as you should already have from arsenal a field internalServiceName
(https://github.com/scality/Arsenal/blob/13f05191a02fcba1be074b64e23e1be30c135137/lib/s3routes/routes.ts#L247C32-L247C51) that has the value backbeat
.
This map mixes action and backend, resourceType
is not enough to know the actual action, you might need to use the method
and the query string operation
. For example metadata has a GET and a PUT.
Maybe you should have 2 fields, one for the backend, one for the action.
This map takes care of the routing and describe all the possible actions:
cloudserver/lib/routes/routeBackbeat.js
Lines 1368 to 1403 in c219cf1
const backbeatRoutes = { | |
PUT: { | |
data: putData, | |
metadata: putMetadata, | |
multiplebackenddata: { | |
putobject: putObject, | |
putpart: putPart, | |
}, | |
}, | |
POST: { | |
multiplebackenddata: { | |
initiatempu: initiateMultipartUpload, | |
completempu: completeMultipartUpload, | |
puttagging: putObjectTagging, | |
}, | |
batchdelete: batchDelete, | |
index: { | |
add: putBucketIndexes, | |
delete: deleteBucketIndexes, | |
}, | |
}, | |
DELETE: { | |
expiration: deleteObjectFromExpiration, | |
multiplebackenddata: { | |
deleteobject: deleteObject, | |
deleteobjecttagging: deleteObjectTagging, | |
abortmpu: abortMultipartUpload, | |
}, | |
}, | |
GET: { | |
metadata: getMetadata, | |
multiplebackendmetadata: headObject, | |
lifecycle: listLifecycle, | |
index: getBucketIndexes, | |
}, | |
}; |
Maybe the action could be added directly from inside each function separately, or use the function name in the routing:
const route = backbeatRoutes[request.method][request.resourceType];
+ log.addDefaultFields({ action: route.name });
return route(request, response, userInfo, log, err => {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the function name sounds good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are many routing branches with conditions, make sure to catch all of them. And there is a divergence in routing in branch 9.1
In 9.0:
- api:
cloudserver/lib/routes/routeBackbeat.js
Line 1535 in db0280e
return backbeatProxy.web(request, response, { target }, err => { - index routes:
cloudserver/lib/routes/routeBackbeat.js
Line 1416 in db0280e
const route = backbeatRoutes[request.method][request.resourceType]; cloudserver/lib/routes/routeBackbeat.js
Lines 1630 to 1638 in db0280e
return backbeatRoutes[request.method][request.resourceType]( request, response, bucketInfo, objMd, log, next); } if (request.resourceType === 'multiplebackendmetadata') { return backbeatRoutes[request.method][request.resourceType]( request, response, log, next); } return backbeatRoutes[request.method][request.resourceType] [request.query.operation](request, response, log, next);
In 9.1:
- api:
cloudserver/lib/routes/routeBackbeat.js
Line 1564 in 38fca1f
return routeBackbeatAPIProxy(request, response, requestContexts, log); - non-object:
cloudserver/lib/routes/routeBackbeat.js
Lines 1503 to 1508 in 38fca1f
if (request.resourceType === 'index') { return routeIndexingAPIs(request, response, userInfo, log, callback); } const route = backbeatRoutes[request.method][request.resourceType]; return route(request, response, userInfo, log, callback); - multiplebackendmetadata
cloudserver/lib/routes/routeBackbeat.js
Lines 1618 to 1625 in 38fca1f
if (useMultipleBackend) { if (request.resourceType === 'multiplebackendmetadata') { return backbeatRoutes[request.method][request.resourceType]( request, response, log, next); } return backbeatRoutes[request.method][request.resourceType] [request.query.operation](request, response, log, next); } cloudserver/lib/routes/routeBackbeat.js
Line 1647 in 38fca1f
return backbeatRoutes[request.method][request.resourceType](
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command:
Alternatively, the |
lib/routes/routeBackbeat.js
Outdated
return authNames; | ||
} | ||
|
||
function routeNameFromRequest(request) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function routeNameFromRequest(request) { | |
function actionFromRequest(request) { |
function routeNameFromRequest(request) { | |
function actionFromRequestRoute(request) { |
/create_integration_branches |
ConflictA conflict has been raised during the creation of I have not created the integration branch. Here are the steps to resolve this conflict: git fetch
git checkout -B w/9.1/bugfix/CLDSRV-746/add-log-fields-to-backbeat-routes-for-analytics origin/development/9.1
git merge origin/bugfix/CLDSRV-746/add-log-fields-to-backbeat-routes-for-analytics
# <intense conflict resolution>
git commit
git push -u origin w/9.1/bugfix/CLDSRV-746/add-log-fields-to-backbeat-routes-for-analytics The following options are set: create_integration_branches |
… the analytics dashboard
b0188f6
to
83691b5
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: create_integration_branches |
/approve |
Build failedThe build for commit did not succeed in branch w/9.1/bugfix/CLDSRV-746/add-log-fields-to-backbeat-routes-for-analytics The following options are set: approve, create_integration_branches |
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
The following branches will NOT be impacted:
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: approve, create_integration_branches |
Queue build failedThe corresponding build for the queue failed:
Remove the pull request from the queue
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue CLDSRV-746. Goodbye fredmnl. |
Adding action, account and userName fields to the backbeat routes.
_normalizeBackbeatRequest
was executed too late as it is the one filling in thebucketName
in therequest
(among other things) and that must happen before this field is used to customize the logger.