SAN-5848 - Stream logs from s3 to the frontend #1906
Conversation
|
Deployed |
lib/socket/common-s3.js
Outdated
| log.trace('called') | ||
|
|
||
| const s3Object = s3.getObject({ | ||
| Bucket: `${process.env.NODE_ENV}.container-logs`, |
There was a problem hiding this comment.
Can you make this whole thing an env. It will help during on-prem installation so we know this is a bucket.
process.env.CONTAINER_LOGS_S3_BUCKET
| log.trace({error}, 'Error while fetching logs from s3') | ||
| cb(error) | ||
| }) | ||
| .on('end', () => { |
There was a problem hiding this comment.
do you need to close createReadStream here? it might be automatic but just making sure no call is needed
There was a problem hiding this comment.
no, the stream is closed when end is called on it already.
| } | ||
| rabbitMQ.publishLogStreamConnected(eventData) | ||
| return commonStream.pipeLogsToClient(destLogStream, baseDataName, tags, data.containerId, { tailLimit }) | ||
| let streamPromise |
There was a problem hiding this comment.
NIT: seems like isolated logic, mind moving it to helper function?
There was a problem hiding this comment.
Specifically the check if the container is running? Or the method to pipe logs to a client? I'm unsure if its needed to be separated.
| }, | ||
| container: { | ||
| dockerContainer: ctx.data.containerId | ||
| dockerContainer: ctx.data.containerId, |
There was a problem hiding this comment.
does unit test cover both cases? where Running is true and false?
There was a problem hiding this comment.
Nope, just when running is true
There was a problem hiding this comment.
can you make test for both cases. and the fallback case? we dont know it works unless its tested :)
| }) | ||
| } | ||
|
|
||
| module.exports = { |
There was a problem hiding this comment.
nit: we have ES6 now, you can make this a class if you want 😉
There was a problem hiding this comment.
Ehh, a little late to the party for streaming.
lib/socket/common-s3.js
Outdated
| log.trace('called') | ||
|
|
||
| const s3Object = s3.getObject({ | ||
| Bucket: `${process.env.NODE_ENV}.container-logs`, |
There was a problem hiding this comment.
Can we change this into a variable? The bucket name should really live in devops-scripts through an ENV so that devop-scripts knows about all S3 buckets created.
There was a problem hiding this comment.
I put it in the .env's
lib/socket/common-s3.js
Outdated
|
|
||
| const pipeLogsToClient = (targetStream, containerId) => { | ||
| const log = logger.child({ | ||
| container: containerId, |
There was a problem hiding this comment.
Nit: Maybe container: containerId, to containerId,
lib/socket/common-s3.js
Outdated
| } | ||
|
|
||
| module.exports = { | ||
| pipeLogsToClient: pipeLogsToClient |
There was a problem hiding this comment.
Nit: pipeLogsToClient: pipeLogsToClient -> pipeLogsToClient
# Conflicts: # configs/.env.test
|
|
We now stream logs to the frontend from s3 buckets. Decision for the architecture can be documented here: https://runnable.atlassian.net/browse/SAN-5848