CLDSRV-750: add Server Access Logs #5980

leif-scality · 2025-10-22T10:07:23Z

No description provided.

bert-e · 2025-10-22T10:07:26Z

Hello leif-scality,

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 · 2025-10-22T10:07:32Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-750 contains:

None

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

9.1.5

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

codecov · 2025-10-22T10:21:42Z

Codecov Report

❌ Patch coverage is 90.42146% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.27%. Comparing base (fad11cb) to head (940992a).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
lib/utilities/serverAccesssLogger.js	88.37%	10 Missing ⚠️
lib/api/bucketPutCors.js	84.21%	3 Missing ⚠️
lib/api/bucketPutWebsite.js	90.47%	2 Missing ⚠️
lib/metadata/metadataUtils.js	92.59%	2 Missing ⚠️
lib/api/bucketDeleteCors.js	93.33%	1 Missing ⚠️
lib/api/bucketDeleteWebsite.js	92.85%	1 Missing ⚠️
lib/api/bucketGetCors.js	92.30%	1 Missing ⚠️
lib/api/bucketGetLogging.js	75.00%	1 Missing ⚠️
lib/api/bucketGetWebsite.js	92.85%	1 Missing ⚠️
lib/api/bucketPutLogging.js	66.66%	1 Missing ⚠️
... and 2 more

Additional details and impacted files

Files with missing lines	Coverage Δ
lib/Config.js	`79.86% <100.00%> (+0.27%)`	⬆️
lib/api/api.js	`91.62% <100.00%> (+0.40%)`	⬆️
lib/api/bucketGetLocation.js	`96.00% <100.00%> (+11.15%)`	⬆️
lib/server.js	`78.14% <100.00%> (+0.86%)`	⬆️
lib/api/bucketDeleteCors.js	`93.54% <93.33%> (-1.33%)`	⬇️
lib/api/bucketDeleteWebsite.js	`93.54% <92.85%> (-1.33%)`	⬇️
lib/api/bucketGetCors.js	`96.42% <92.30%> (+13.09%)`	⬆️
lib/api/bucketGetLogging.js	`96.87% <75.00%> (-3.13%)`	⬇️
lib/api/bucketGetWebsite.js	`96.42% <92.85%> (+13.09%)`	⬆️
lib/api/bucketPutLogging.js	`95.65% <66.66%> (-2.03%)`	⬇️
... and 6 more

... and 1 file with indirect coverage changes

@@                 Coverage Diff                 @@
##           development/9.1    #5980      +/-   ##
===================================================
+ Coverage            84.10%   84.27%   +0.17%     
===================================================
  Files                  193      194       +1     
  Lines                12335    12422      +87     
===================================================
+ Hits                 10374    10469      +95     
+ Misses                1961     1953       -8

Flag	Coverage Δ
ceph-backend-test	`65.06% <86.20%> (+0.27%)`	⬆️
file-ft-tests	`67.24% <86.20%> (+0.24%)`	⬆️
kmip-ft-tests	`27.77% <52.87%> (+0.64%)`	⬆️
mongo-v0-ft-tests	`68.52% <86.20%> (+0.20%)`	⬆️
mongo-v1-ft-tests	`68.54% <86.20%> (+0.23%)`	⬆️
multiple-backend	`34.94% <56.70%> (+0.63%)`	⬆️
sur-tests	`36.12% <54.78%> (+0.61%)`	⬆️
sur-tests-inflights	`37.18% <54.78%> (+0.57%)`	⬆️
unit	`69.28% <57.08%> (-0.24%)`	⬇️
utapi-v2-tests	`34.08% <54.78%> (+0.62%)`	⬆️

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

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

bert-e · 2025-11-06T15:14:55Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-750 contains:

None

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

9.1.6

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

francoisferrand · 2025-11-07T15:53:11Z

@nicolas2bert @leif-scality Is this supposed to land in a patch release on 9.1 ?

nicolas2bert · 2025-11-10T10:18:05Z

lib/Config.js


+function parseServerAccessLogs(config) {
+    const res = {
+        enabled: true,


Should it be false by default?

Yes, let's set it to false by default so that access.log is not created in Artesca.

nicolas2bert · 2025-11-10T10:21:32Z

config.json

        "objectPutRetention": true
+    },
+    "serverAccessLogs": {
+        "enabled": true,


Can we also make sure the API relies on this toggle and returns Not Implemented if enabled: false?

bert-e · 2025-11-10T10:21:45Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-750 contains:

None

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

9.1.6
9.2.0

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

dvasilas

(partial review, but I have some comments that can be addressed)

What about tests?
I think we should have tests that verify that what gets written to server-access.log is what is expected, for every API.
It is ok for me to add tests separately in another PR if you prefer, we just need to create the ticket so that we don't forget.

dvasilas · 2025-11-11T16:03:04Z

lib/Config.js

+function parseServerAccessLogs(config) {
+    const res = {
+        enabled: true,
+        outputFile: './logs/server-access.log',


Suggested change

outputFile: './logs/server-access.log',

outputFile: '/logs/server-access.log',

dvasilas · 2025-11-11T16:08:04Z

lib/Config.js


+function parseServerAccessLogs(config) {
+    const res = {
+        enabled: true,


Yes, let's set it to false by default so that access.log is not created in Artesca.

dvasilas · 2025-11-11T16:22:39Z

lib/server.js

+        res.on('finish', () => {
+            // eslint-disable-next-line no-param-reassign
+            req.serverAccessLog.endTime = process.hrtime.bigint();
+            logServerAccess(req.serverAccessLog, req, res);


We log every request, and we end up logging healthcheck requests

{"name":"ServerAccessLogger","time":1762877454094,"action":null,"accountName":null,"accountDisplayName":null,"userName":null,"clientPort":46630,"httpMethod":"GET","bytesDeleted":null,"bytesReceived":0,"bodyLength":0,"contentLength":0,"elapsed_ms":16.621336,"httpURL":"/_/healthcheck/deep","startTime":"14621972622003","requester":null,"operation":"REST.GET.UNKNOWN","requestURI":"GET /_/healthcheck/deep HTTP/1.1","errorCode":null,"objectSize":null,"totalTime":"16","turnAroundTime":null,"referer":null,"userAgent":"Wget/1.24.5","versionID":null,"signatureVersion":null,"cipherSuite":null,"authenticationType":null,"hostHeader":"10.160.105.56","tlsVersion":null,"aclRequired":null,"bucketOwner":null,"bucketName":null,"req_id":"7e243b2d294f92a03ca9","bytesSent":null,"clientIP":"::ffff:127.0.0.1","httpCode":200,"objectKey":null,"logFormatVersion":"0","loggingEnabled":false,"loggingTargetBucket":null,"loggingTargetPrefix":null,"awsAccessKeyID":null,"raftSessionID":null,"level":"info","message":"SERVER_ACCESS_LOG","hostname":"dvasilas-L01-store-1","pid":89}

One idea to avoid that could be not to write to access log at all for internal routes, using the url to detect them:

if (!req.url.startsWith('/_/')) { res.on('finish', () => { ... }

That way we wil also filter out requests to the backbeat routes.

dvasilas · 2025-11-13T09:32:10Z

config.json

        "objectPutRetention": true
+    },
+    "serverAccessLogs": {
+        "enabled": false,


Could we have two types of "enabled"?

"Are we writing the server-access.log" (default false for Artesca). We could call this field "generateAcessLogFile"

"Is the feature enabled in Federation" (false by default, will be configured by Federation). When this is false: (1) The API returns not implemented, and (2) Cloudserver always sets loggingEnabled to false. That way, we can disable the feature in Federation.

In what case we would generate the file and not write to it?
can the config file be modified in Federation?

In what case we would generate the file and not write to it?

For Artesca we should not generate it and not write to it

can the config file be modified in Federation?

Yes, the config.json in this repo is not actually used in Federation.
Federation has a template to generate its own: https://github.com/scality/Federation/blob/development/9.5/roles/run-s3/templates/config.json.j2
( so when bumping cloudserver, we should also update this template)

For Artesca we should not generate it and not write to it

This is already the case when enabled = false

leif-scality force-pushed the improvement/CLDSRV-750-write-server-access-logs branch from 2e9e717 to 9404507 Compare October 22, 2025 10:09

leif-scality force-pushed the improvement/CLDSRV-750-write-server-access-logs branch 3 times, most recently from 00832f8 to a63c29f Compare October 23, 2025 20:52

leif-scality force-pushed the improvement/CLDSRV-750-write-server-access-logs branch from ed7705b to b95e090 Compare November 4, 2025 14:40

leif-scality force-pushed the improvement/CLDSRV-750-write-server-access-logs branch from 67a4483 to 9ecbb31 Compare November 6, 2025 15:22

leif-scality marked this pull request as ready for review November 7, 2025 15:45

leif-scality force-pushed the improvement/CLDSRV-750-write-server-access-logs branch from 6c4b7ec to 5cf2562 Compare November 7, 2025 15:48

nicolas2bert reviewed Nov 10, 2025

View reviewed changes

dvasilas reviewed Nov 11, 2025

View reviewed changes

dvasilas reviewed Nov 13, 2025

View reviewed changes

leif-scality added 4 commits November 13, 2025 15:33

CLDSRV-750: add Server Access Logs

94934ea

CLDSRV-750: replace repeated logic with standardMetadataValidateBucket

42c1689

CLDSRV-750: add raftSessionID to server access logs

d264a9e

CLDSRV-750: add analytics fields to server access logs

60e7720

leif-scality force-pushed the improvement/CLDSRV-750-write-server-access-logs branch 2 times, most recently from 4f7ce85 to 85d7ab9 Compare November 13, 2025 16:40

leif-scality added 3 commits November 14, 2025 15:26

CLDSRV-750: skip server access logs for heatbeat and backbeat routes

757c15b

CLDSRV-750: disable Put/GetBucketLogging if it is disabled in the config

9397b79

tmp arsenal branch

940992a

leif-scality force-pushed the improvement/CLDSRV-750-write-server-access-logs branch from 81eb964 to 940992a Compare November 14, 2025 14:26

	outputFile: './logs/server-access.log',
	outputFile: '/logs/server-access.log',

CLDSRV-750: add Server Access Logs #5980

Are you sure you want to change the base?

CLDSRV-750: add Server Access Logs #5980

Uh oh!

Conversation

leif-scality commented Oct 22, 2025

Uh oh!

bert-e commented Oct 22, 2025

Hello leif-scality,

Uh oh!

bert-e commented Oct 22, 2025

Incorrect fix version

Uh oh!

codecov bot commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bert-e commented Nov 6, 2025

Incorrect fix version

Uh oh!

francoisferrand commented Nov 7, 2025

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!

bert-e commented Nov 10, 2025

Incorrect fix version

Uh oh!

dvasilas 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented Oct 22, 2025 •

edited

Loading