-
Notifications
You must be signed in to change notification settings - Fork 20
ARSN-531: record server access log fields in s3routes #2554
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
base: development/8.2
Are you sure you want to change the base?
ARSN-531: record server access log fields in s3routes #2554
Conversation
Hello leif-scality,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
99b4780 to
7176cb3
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development/8.2 #2554 +/- ##
===================================================
- Coverage 71.40% 71.36% -0.04%
===================================================
Files 221 221
Lines 17816 17865 +49
Branches 3681 3691 +10
===================================================
+ Hits 12721 12750 +29
- Misses 5091 5111 +20
Partials 4 4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
d1b8054 to
77e1009
Compare
5ee5876 to
17459f7
Compare
lib/s3routes/routesUtils.ts
Outdated
| const contentLength = request.headers['x-amz-decoded-content-length'] ? | ||
| request.headers['x-amz-decoded-content-length'] : | ||
| request.headers['content-length']; | ||
| request.headers['content-length']; // here |
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.
| request.headers['content-length']; // here | |
| request.headers['content-length']; |
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.
removed
lib/s3routes/routesUtils.ts
Outdated
| // called if 'end' is. | ||
| currentStream = readable; | ||
|
|
||
| storeServerAccessLogFields(response, process.hrtime.bigint()); |
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.
I think this is not in the correct place.
It is inside the eachSeries for location, so if there are multiple locations, it will be called multiple times.
It may be better to place it inside:
if (!response.headersSent) {
response.writeHead(httpCodeOnSucess);
}
`
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.
Also (more important), shouldn't this include bytesSent? It is used by objectGet.
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.
fixed, calculated the bytesSent using _computeContentLengthFromLocation
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.
retrieveDataAzure is out of context, but you could add a TODO comment saying that it does not capture log access log fields in case this feature is ever used in Artesca.
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.
added the comment
17459f7 to
d16b7b7
Compare
d16b7b7 to
6c46d6c
Compare
Extract fields to be used by cloudserver to write the server access logs