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

startup logging #3380

Merged
merged 4 commits into from
Nov 25, 2021
Merged

startup logging #3380

merged 4 commits into from
Nov 25, 2021

Conversation

coot
Copy link
Contributor

@coot coot commented Nov 22, 2021 •
edited
Loading

  • Added StartupTracer
  • Use StartupTracer to log start time and db validation startup
  • Added a todo

@coot coot requested review from deepfire and jutaro November 22, 2021 14:38
@coot coot force-pushed the coot/startup-info branch from 196f71d to 84635d3 Compare November 22, 2021 14:43
deepfire
deepfire approved these changes Nov 22, 2021
View reviewed changes
Copy link
Contributor

@deepfire deepfire left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you @coot !

@coot coot marked this pull request as draft November 22, 2021 18:00
@coot coot force-pushed the coot/startup-info branch from 84635d3 to f914dcf Compare November 22, 2021 19:51
@coot coot marked this pull request as ready for review November 22, 2021 19:51
@coot
Copy link
Contributor Author

coot commented Nov 22, 2021

@deepfire I added proper json instances, and better pretty printing, I added proper severity levels, and I checked that it actually works.

@coot coot force-pushed the coot/startup-info branch from f914dcf to 302cbf8 Compare November 22, 2021 23:02
karknu
karknu approved these changes Nov 23, 2021
View reviewed changes
Copy link
Contributor

@karknu karknu left a comment

Choose a reason for hiding this comment

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

LGTM.

cardano-node/src/Cardano/Tracing/Startup.hs Show resolved Hide resolved
@coot coot force-pushed the coot/startup-info branch 4 times, most recently from b4407d1 to 9c6cd97 Compare November 23, 2021 14:27
jutaro
jutaro approved these changes Nov 23, 2021
View reviewed changes
Copy link
Contributor

@jutaro jutaro left a comment

Choose a reason for hiding this comment

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

LGTM

@coot coot force-pushed the coot/startup-info branch 3 times, most recently from 7489400 to 3bf889b Compare November 24, 2021 17:24
@deepfire
Copy link
Contributor

bors r+

iohk-bors bot added a commit that referenced this pull request Nov 24, 2021
@iohk-bors @coot
Merge #3380
eac0c61 
3380: startup logging r=deepfire a=coot

- Added StartupTracer
- Use StartupTracer to log start time and db validation startup
- Added a todo


Co-authored-by: Marcin Szamotulski <profunctor@pm.me>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Nov 24, 2021

This PR was included in a batch that successfully built, but then failed to merge into master. It will not be retried.

Additional information:

{"message":"Waiting on code owner review from DavidEichmann, dcoutts, erikd, JaredCorduan, Jimbo4350, nc6, and/or newhoggy.","documentation_url":"https://docs.github.com/articles/about-protected-branches"}

@deepfire
Copy link
Contributor

bors r+

iohk-bors bot added a commit that referenced this pull request Nov 25, 2021
@iohk-bors @coot
Merge #3380
46bf35f 
3380: startup logging r=deepfire a=coot

- Added StartupTracer
- Use StartupTracer to log start time and db validation startup
- Added a todo


Co-authored-by: Marcin Szamotulski <profunctor@pm.me>
newhoggy
newhoggy reviewed Nov 25, 2021
View reviewed changes
cardano-node/src/Cardano/Tracing/Startup.hs
(map (\(v, bv) -> show v ++ "\t" ++ show bv)
. Map.assocs
$ supportedNodeToCleintVersions)
]
Copy link
Contributor

@newhoggy newhoggy Nov 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Seems like a lot of string appends. Do we need to worry about performance?

Similarly for other code below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, not at all; this runs once at startup.

@newhoggy
Copy link
Contributor

I'm assuming both TODOs are intentional.

newhoggy
newhoggy approved these changes Nov 25, 2021
View reviewed changes
Copy link
Contributor

@newhoggy newhoggy left a comment

Choose a reason for hiding this comment

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

Left some comments. Otherwise LGTM.

coot added 4 commits November 25, 2021 13:15
@coot
Added StartupTracer
72955a7 
Use a standard Tracer to log information when node starts.
@coot
Use StartupTracer to log start time and db validation on startup
ba755fd
@coot
Added WarnDevelopmentNetworkProtocols warning
fb7583b 
Warn the user which development protocols were enabled with
'TestEnableDevelopmentNetworkProtocols` option.
@coot
Added a todo
2201fa9
@coot coot force-pushed the coot/startup-info branch from 3bf889b to 2201fa9 Compare November 25, 2021 12:16
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Nov 25, 2021

Canceled.

@deepfire
Copy link
Contributor

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Nov 25, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit 8a59251 into master Nov 25, 2021
@iohk-bors iohk-bors bot deleted the coot/startup-info branch November 25, 2021 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karknu karknu karknu approved these changes

@jutaro jutaro jutaro approved these changes

@deepfire deepfire deepfire approved these changes

@newhoggy newhoggy newhoggy approved these changes

@DavidEichmann DavidEichmann Awaiting requested review from DavidEichmann

@dcoutts dcoutts Awaiting requested review from dcoutts

@erikd erikd Awaiting requested review from erikd

@JaredCorduan JaredCorduan Awaiting requested review from JaredCorduan

@Jimbo4350 Jimbo4350 Awaiting requested review from Jimbo4350

@nc6 nc6 Awaiting requested review from nc6

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

Successfully merging this pull request may close these issues.
5 participants
@coot @deepfire @newhoggy @jutaro @karknu