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

Detect ansi clear lines and add ansi timestamps #1128

Merged
merged 2 commits into from
Dec 5, 2019

Conversation

lox
Copy link
Contributor

@lox lox commented Nov 26, 2019

Currently the ANSI timestamps don't show up certain output (like docker) that uses creative ANSI control characters for cursor movements.

This detects things like \x1b[K which clears the current line and includes the ANSI timestamp technology.

@keithpitt
Copy link
Member

<3<3<3

@keithpitt keithpitt requested a review from a team November 26, 2019 06:07
Copy link
Member

@keithpitt keithpitt left a comment

Choose a reason for hiding this comment

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

I can't speak to the code, but it has tests, and it solves a long running issue in the agent that I'm very very very excited to see fixed. So it get's a 👍 from me!

Maybe someone from @buildkite/golang has some feels though on any potential issues?

@keithpitt
Copy link
Member

For further context, currently Buildkite can't show line timestamps like here:

image

This change will allow us to show timestamps on each of those lines.

@lox
Copy link
Contributor Author

lox commented Nov 26, 2019

It needs some more testing, I suspect there are some more escape codes that we need to handle. There are some edge cases that I don't handle too.

// loop through line breaks and add prefix
for l := len(data); offset < l; {
// find either a newline or an escape char
next := bytes.IndexAny(data[offset:], "\n\x1b")
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to standardise on hex \x1b or octal \033 for escape chars in the codebase/docs/etc.
Currently there’s a bit of both in our codebase. Octal is a ridiculous radix. So this \x1b is good. Basically ignore this comment.

for offset < len(data) {
next := bytes.IndexRune(data[offset:], '\n')
// loop through line breaks and add prefix
for l := len(data); offset < l; {
Copy link
Member

Choose a reason for hiding this comment

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

Would a bufio.Scanner be appropriate here, with a custom split function detecting new line or escape?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Programs that need more control over error handling or large tokens, or must run sequential scans on a reader, should use bufio.Reader instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bufio.Scanner fails on long lines, it's a pain.

@keithpitt
Copy link
Member

Can we merge and see how it goes?

@lox
Copy link
Contributor Author

lox commented Nov 29, 2019

I’ll test it now.

@keithpitt
Copy link
Member

I fixed the last remaining bug in the UI for this, so I'm keen to get this merged in.

@lox lox merged commit c7759b2 into master Dec 5, 2019
@yob yob deleted the add-ansi-timestamp-to-escape-chars branch October 8, 2020 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants