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

feat: standardize logging #1302

Merged
merged 51 commits into from
Jan 25, 2024
Merged

feat: standardize logging #1302

merged 51 commits into from
Jan 25, 2024

Conversation

zivkovicmilos
Copy link
Member

@zivkovicmilos zivkovicmilos commented Oct 26, 2023

Description

This PR introduces log standardization across the codebase, to utilize the slog.Logger.

Since the gno repo currently utilizes go1.20 as the minimum version, this PR utilizes the golang.org/x/exp/log package.

Additionally, this PR removes log level management from the TM2 config, and instead moves it into the start command.

Closes #999

Thank you @gfanton for helping out with zap and simplifying the API 🙏
Special shoutout to @moul's zapconfig, for being an inspiration on zap setup.

BREAKING CHANGE:

  • the tm2/log logger interface has been removed, everything has been swapped to *slog.Logger

Example of different log levels:
cast

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

@zivkovicmilos zivkovicmilos added the 🌱 feature New update to Gno label Oct 26, 2023
@github-actions github-actions bot added 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Oct 26, 2023
@zivkovicmilos
Copy link
Member Author

@gfanton @ajnavarro

Things we need to agree on:

  • logging creation API (should it be able to error out?)
  • default log levels
  • how do we manage test loggers? what's the output / level?
  • ... ?

@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

Attention: 103 lines in your changes are missing coverage. Please review.

Comparison is base (87179b9) 55.80% compared to head (18727d8) 56.05%.
Report is 1 commits behind head on master.

Files Patch % Lines
tm2/pkg/log/testing.go 0.00% 49 Missing ⚠️
gno.land/pkg/integration/testing_integration.go 38.09% 12 Missing and 1 partial ⚠️
tm2/pkg/log/noop.go 0.00% 12 Missing ⚠️
gno.land/pkg/gnoland/app.go 0.00% 5 Missing ⚠️
gno.land/cmd/gnoland/start.go 85.18% 3 Missing and 1 partial ⚠️
gno.land/cmd/gnoweb/main.go 0.00% 3 Missing ⚠️
tm2/pkg/sdk/context.go 33.33% 2 Missing ⚠️
gno.land/pkg/gnoland/node_inmemory.go 0.00% 1 Missing ⚠️
gno.land/pkg/gnoweb/gnoweb.go 93.33% 1 Missing ⚠️
...pkg/bft/abci/example/kvstore/persistent_kvstore.go 50.00% 1 Missing ⚠️
... and 12 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1302      +/-   ##
==========================================
+ Coverage   55.80%   56.05%   +0.24%     
==========================================
  Files         435      436       +1     
  Lines       66056    66498     +442     
==========================================
+ Hits        36863    37274     +411     
- Misses      26320    26325       +5     
- Partials     2873     2899      +26     
Flag Coverage Δ
go-1.21.x ∅ <ø> (∅)
misc ∅ <ø> (∅)
misc-_test.genstd ∅ <ø> (∅)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zivkovicmilos zivkovicmilos added the breaking change Functionality that contains breaking changes label Jan 15, 2024
@zivkovicmilos
Copy link
Member Author

Pinging @moul to take a look at the discussions 🙏

@zivkovicmilos
Copy link
Member Author

I've added a small go mod tidy script:

6471c93

@zivkovicmilos
Copy link
Member Author

@moul

@gfanton has enabled the support for debug logs in the CI, so now we can grab CI log artifacts on a per-test basis 😎

Screenshot 2024-01-23 at 17 41 17

Screenshot 2024-01-23 at 17 55 21

Makefile Outdated
@@ -45,3 +45,13 @@ fmt:
.PHONY: lint
lint:
$(rundep) github.com/golangci/golangci-lint/cmd/golangci-lint run --config .github/golangci.yml

.PHONY: imports
Copy link
Member

Choose a reason for hiding this comment

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

can we merge in make fmt instead?

Copy link
Member Author

@zivkovicmilos zivkovicmilos Jan 24, 2024

Choose a reason for hiding this comment

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

Merged:

7eb92d2

Updated CI:
7720593

We should think about goimports linting for *.gno imports in the future

.PHONY: tidy
tidy:
$(MAKE) --no-print-directory -C misc tidy
Copy link
Member

Choose a reason for hiding this comment

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

Can you please double-check? I vaguely recall creating something similar, possibly only in the CI. Just ensure that there isn't already an existing solution for the same purpose.

Copy link
Member Author

Choose a reason for hiding this comment

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

It turns out we had not 1, but 2 tidy flows 🙂

Fixed:
9ab96d0 and a8fe8d3

@moul moul merged commit da35d3c into master Jan 25, 2024
198 checks passed
@moul moul deleted the feat/log branch January 25, 2024 17:38
thehowl pushed a commit that referenced this pull request Jan 25, 2024
## Description

This PR fixes the `noop` log reference that was broken in
#1574, after the #1302 PR was merged
in earlier.

<details><summary>Contributors' checklist...</summary>

- [x] Added new tests, or not needed, or not feasible
- [x] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [x] Updated the official documentation or not needed
- [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [x] Added references to related issues and PRs
- [ ] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>
leohhhn pushed a commit to leohhhn/gno that referenced this pull request Jan 31, 2024
## Description

This PR introduces log standardization across the codebase, to utilize
the `slog.Logger`.

Since the gno repo currently utilizes `go1.20` as the minimum version,
this PR utilizes the `golang.org/x/exp/log` package.

Additionally, this PR removes log level management from the TM2 config,
and instead moves it into the start command.

Closes gnolang#999 

Thank you @gfanton for helping out with zap and simplifying the API 🙏 
Special shoutout to @moul's
[zapconfig](https://github.com/moul/zapconfig/tree/master), for being an
inspiration on zap setup.

**BREAKING CHANGE**:
- the `tm2/log` logger interface has been removed, everything has been
swapped to `*slog.Logger`

Example of different log levels:

![cast](https://github.com/gnolang/gno/assets/16712663/2288ad98-7f72-4f05-b749-827b815b7f16)


<details><summary>Contributors' checklist...</summary>

- [x] Added new tests, or not needed, or not feasible
- [x] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [x] Updated the official documentation or not needed
- [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [ ] Added references to related issues and PRs
- [ ] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>

---------

Signed-off-by: gfanton <[email protected]>
Co-authored-by: gfanton <[email protected]>
Co-authored-by: Manfred Touron <[email protected]>
leohhhn pushed a commit to leohhhn/gno that referenced this pull request Jan 31, 2024
## Description

This PR fixes the `noop` log reference that was broken in
gnolang#1574, after the gnolang#1302 PR was merged
in earlier.

<details><summary>Contributors' checklist...</summary>

- [x] Added new tests, or not needed, or not feasible
- [x] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [x] Updated the official documentation or not needed
- [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [x] Added references to related issues and PRs
- [ ] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Functionality that contains breaking changes 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related 📦 🤖 gnovm Issues or PRs gnovm related 🧾 package/realm Tag used for new Realms or Packages. 🌱 feature New update to Gno
Projects
Status: Done
Status: Done
Status: 🌟 Wanted for Launch
Archived in project
Development

Successfully merging this pull request may close these issues.

META Improve Logging
5 participants