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

Dev Tools: Integrate Logfile Navigator (lnav) #5000

Merged
merged 5 commits into from Mar 24, 2023

Conversation

Aharonee
Copy link
Contributor

@Aharonee Aharonee commented Jan 11, 2023

  • Install lnav (Logfile Navigator) dependency and add custom format for logrus
  • Reformat "Logging Starting" messages to be JSON instead of strings

Usage:

  1. Install dependency and custom format by running ./scripts/configure_dev.sh
  2. Open logfile by running lnav node.log

@Aharonee Aharonee self-assigned this Jan 11, 2023
@Aharonee Aharonee changed the title Integrate Logfile Navigator (lnav) Tools: Integrate Logfile Navigator (lnav) Jan 11, 2023
@Aharonee Aharonee changed the title Tools: Integrate Logfile Navigator (lnav) Dev Tools: Integrate Logfile Navigator (lnav) Jan 11, 2023
@codecov
Copy link

codecov bot commented Jan 11, 2023

Codecov Report

Merging #5000 (f5aef89) into master (19fa6c2) will decrease coverage by 1.95%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #5000      +/-   ##
==========================================
- Coverage   53.65%   51.71%   -1.95%     
==========================================
  Files         432      432              
  Lines       54058    54058              
==========================================
- Hits        29007    27958    -1049     
- Misses      22805    23783     +978     
- Partials     2246     2317      +71     
Impacted Files Coverage Δ
daemon/algod/server.go 4.62% <0.00%> (ø)

... and 103 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

daemon/algod/server.go Outdated Show resolved Hide resolved
Copy link
Contributor

@cce cce left a comment

Choose a reason for hiding this comment

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

I would prefer to see the logging started, telemetry enabled vs disabled, GUID/Session information show up in node.log, with some structure show up node.log, rather than remove it altogether

@Aharonee Aharonee requested a review from cce January 11, 2023 16:04
"title" : "Algornad Node Log Format",
"description" : "Log format for logrus, used by go-algorand.",
"url" : "https://github.com/sirupsen/logrus",
"file-pattern": "node.log",
Copy link
Contributor

Choose a reason for hiding this comment

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

node archive is missing

Copy link
Contributor Author

@Aharonee Aharonee Jan 12, 2023

Choose a reason for hiding this comment

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

file-pattern is used to avoid potential conflicts when lnav chooses the file format to use.
I've replaced it with sample which defines some log line samples and uses them for conflict resolution instead (seems like this is the more common approach).

bbroder-algo
bbroder-algo previously approved these changes Mar 14, 2023
Copy link
Contributor

@bbroder-algo bbroder-algo left a comment

Choose a reason for hiding this comment

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

I am in favor of making the log entirely one object format.

Co-authored-by: Hang Su <87964331+ahangsu@users.noreply.github.com>
Copy link
Contributor

@ahangsu ahangsu left a comment

Choose a reason for hiding this comment

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

let me second an approval, for it helps in reading the logs

@algorandskiy
Copy link
Contributor

I'd prefer to see a some kind of lnav header as Chris suggested as well but LGTM

Comment on lines +145 to +146
s.log.Infoln("++++++++++++++++++++++++++++++++++++++++")
s.log.Infoln("Logging Starting")
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems weird to print "Logging Started" in the log file.

Some better options:

  • Write "Logging Started: " to stdout.
  • Delete these lines.

Either way, definitely remove the ++++++++++++++++++++++++++++++++++++++++ banner.

Copy link
Contributor

Choose a reason for hiding this comment

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

+++ATH0

Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm restarting a container, and tailing logs, it is helpful to see a clear location where the restart is obvious. But that said, it shouldn't deviate from the normal formatting expected by machine parsers, e.g. Filebeat.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe, "algod process start"

Copy link
Contributor

Choose a reason for hiding this comment

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

@winder winder self-requested a review March 24, 2023 15:46
@winder winder merged commit 88cf74a into algorand:master Mar 24, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants