-
Notifications
You must be signed in to change notification settings - Fork 2k
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
UI: Log streaming #3564
UI: Log streaming #3564
Conversation
ui/app/utils/classes/log.js
Outdated
}; | ||
|
||
if (window.ReadableStream) { | ||
this.set('logStreamer', StreamLogger.create(args)); |
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.
Maybe have a StreamLogger.isSupported()
method so you don't have to use window.ReadableStream
everywhere. Then in create()
you can also assert that it's supported.
ui/app/utils/classes/log.js
Outdated
this.stop(); | ||
let text = yield logFetch(url).then(res => res.text()); | ||
|
||
if (text.length > 50000) { |
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.
Counting newlines might be more fair and/or fit your window better but I can see why you'd also do this instead.
ui/app/utils/classes/poll-logger.js
Outdated
} else { | ||
additionalParams = { | ||
origin: 'end', | ||
offset: 50000, |
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 wonder if you might wanna throw 50000
in a config file since it's used in several spots.
@@ -20,7 +20,10 @@ | |||
"prettier --single-quote --trailing-comma es5 --print-width 100 --write", | |||
"git add" | |||
], | |||
"ui/app/styles/**/*.*": ["prettier --write", "git add"] | |||
"ui/app/styles/**/*.*": [ |
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.
npm/yarn and prettier always fight the formatting of these lines. I try to make prettier win since that's easier to keep up with.
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.
A few comments. I noticed that some tests are failing.
Supports: - Reading the head of a log - Reading the tail of a log - Following a log in one of two ways: - Streaming the HTTP request (using fetch res.getReader) - Polling the log endpoint (using EC timeouts)
Just verifying the log code works
Until they can be mocked, they can't be used
This will need to get better for testing the offset stitching logic in the polling case.
7e41b6b
to
d930180
Compare
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.
good stuff!
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
:o
Supports:
In order to not destroy browsers, the log will only show 50k characters. When streaming or reading the tail, anything before the last 50k characters will be truncated. When reading the head, anything after the first 50k characters will be truncated.
Having scroll back is nice, and I want to make this better, but I think this is a good first iteration.