Skip to content
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

Merged
merged 25 commits into from
Nov 29, 2017
Merged

UI: Log streaming #3564

merged 25 commits into from
Nov 29, 2017

Conversation

DingoEatingFuzz
Copy link
Contributor

:o

tail-logs-complete

Supports:

  • Reading the head
  • Reading the tail
  • Streaming (true streaming in supported browsers, polling every second in unsupported browsers).
  • Switching between stdout and stderr

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.

};

if (window.ReadableStream) {
this.set('logStreamer', StreamLogger.create(args));
Copy link
Member

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.

this.stop();
let text = yield logFetch(url).then(res => res.text());

if (text.length > 50000) {
Copy link
Member

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.

} else {
additionalParams = {
origin: 'end',
offset: 50000,
Copy link
Member

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/**/*.*": [
Copy link
Member

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.

Copy link
Member

@thrashr888 thrashr888 left a 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.
Copy link
Member

@thrashr888 thrashr888 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good stuff!

@DingoEatingFuzz DingoEatingFuzz merged commit e369a9a into master Nov 29, 2017
@DingoEatingFuzz DingoEatingFuzz deleted the f-ui-log-streaming branch November 29, 2017 17:36
@github-actions
Copy link

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants