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.

Sign up for GitHub

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

Jump to bottom

feat: add —log-timestamps option #1025

Merged
merged 6 commits into from
Mar 6, 2024
Merged

feat: add —log-timestamps option #1025

merged 6 commits into from
Mar 6, 2024

Conversation

matt2e
Copy link
Collaborator

@matt2e matt2e commented Mar 6, 2024 •
edited
Loading

#1009
Adding add --log-timestamps include timestamps in terminal logs

Example

16:26:20 info: Starting FTL with 1 controller(s) and 0 runner(s)
16:26:20 warn: Detected change in +/Users/mtoohey/Code/ftl/examples/go/echo/go.sum, rebuilding...
16:26:20 error: Error deploying module /Users/mtoohey/Code/ftl/examples/go/echo. Will retry: module "time" not found
16:26:20 warn: Detected change in +/Users/mtoohey/Code/ftl/examples/go/time/time.go, rebuilding...
16:26:20 info:time: Building module

@alecthomas alecthomas mentioned this pull request Mar 6, 2024
Open
alecthomas
alecthomas approved these changes Mar 6, 2024
View reviewed changes
Copy link
Collaborator

@alecthomas alecthomas left a comment

Choose a reason for hiding this comment

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

Nice.

internal/log/configure.go Outdated
JSON bool `help:"Log in JSON format." env:"LOG_JSON"`
Level Level `help:"Log level." default:"info" env:"LOG_LEVEL"`
JSON bool `help:"Log in JSON format." env:"LOG_JSON"`
Timestamps bool `help:"Include relative timestamps in logs." env:"LOG_TIMESTAMPS"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be good to clarify that this is only in plain text logs, not JSON logs.

@alecthomas
Copy link
Collaborator

cc @lamchau

@lamchau
Copy link

lamchau commented Mar 6, 2024

awesome, thanks for this!

alecthomas
alecthomas reviewed Mar 6, 2024
View reviewed changes
internal/log/plain.go Outdated
case d < time.Minute:
timestamp = fmt.Sprintf("%ds", d/time.Second)
case d < time.Hour:
timestamp = fmt.Sprintf("%dm", d/time.Minute)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mmmm so I think this minute/hour granularity will not be super useful in practice. The rationale is that you will still want to be able to tell the relative difference between two log entries...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't really have a great idea on how to format that though, to be honest.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

     999ms
       20s
   10m 30s
1h 10m 30s

This could be more useful?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be okay to drop the spaces.

Though I wonder if at this point it shouldn't just be:

  0.999
     20
1:10:30

WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

1h10m30s feels cramped to me in a way that 1:10:30 doesn't

Copy link
Collaborator

Choose a reason for hiding this comment

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

I do tend to agree 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe even 0:20 for seconds. A number by itself is a bit contextless

  0.999
   0:20
1:10:30

Copy link
Collaborator

Choose a reason for hiding this comment

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

At this point maybe we should just use the 24h time rather than relative?

16:07:23 info: foo
16:09:01 debug: bar

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thinking it through... I think relative only makes sense if we mostly cared about startup times, but engineers using ftl would have ftl dev running in the background through their day right? And when they look back at the logs the relative timestamps will be in the hours and not be easily convertible to "oh that was 30 mins ago", where as with non relative times it'll be pretty intuitive

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated the code to 24h timestamps

alecthomas
alecthomas approved these changes Mar 6, 2024
View reviewed changes
Copy link
Collaborator

@alecthomas alecthomas left a comment

Choose a reason for hiding this comment

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

LGTM

@matt2e matt2e merged commit 1a713ef into main Mar 6, 2024
11 checks passed
@matt2e matt2e deleted the matt2e/log-timestamps branch March 6, 2024 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alecthomas alecthomas alecthomas approved these changes

@dhanji dhanji Awaiting requested review from dhanji

@wesbillman wesbillman Awaiting requested review from wesbillman

@worstell worstell Awaiting requested review from worstell

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@matt2e @alecthomas @lamchau