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

Ability to change log verbosity while the node is running #799

Merged
merged 11 commits into from
Aug 19, 2024

Conversation

MakisChristou
Copy link
Member

@MakisChristou MakisChristou commented Jul 25, 2024

This PR adds a new server running on a separate port (default 2113) bound to localhost and disabled by default. It implements a route /admin and uses query parameters to get the new verbosity level. To use it you can run thor with the following arguments:

./thor solo --on-demand --enable-admin

and use the following curl command to change the verbosity

curl "http://localhost:2113/admin?verbosity=debug"

The available options are debug, info, warn, error, trace, crit

@codecov-commenter
Copy link

codecov-commenter commented Jul 25, 2024

Codecov Report

Attention: Patch coverage is 5.55556% with 119 lines in your changes missing coverage. Please review.

Project coverage is 62.18%. Comparing base (8ba7f54) to head (1aae362).

Files Patch % Lines
admin/admin.go 0.00% 53 Missing ⚠️
cmd/thor/utils.go 0.00% 35 Missing ⚠️
cmd/thor/main.go 0.00% 22 Missing ⚠️
cmd/disco/utils.go 0.00% 5 Missing ⚠️
log/handler.go 63.63% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #799      +/-   ##
==========================================
- Coverage   62.53%   62.18%   -0.36%     
==========================================
  Files         203      204       +1     
  Lines       18731    18840     +109     
==========================================
+ Hits        11713    11715       +2     
- Misses       5920     6027     +107     
  Partials     1098     1098              

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

admin/admin.go Outdated Show resolved Hide resolved
Copy link
Member

@otherview otherview 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 - ooc is there a way that we can test this feature on unit tests ?

admin/admin.go Outdated Show resolved Hide resolved
cmd/thor/flags.go Show resolved Hide resolved
cmd/disco/utils.go Show resolved Hide resolved
docs/hosting-a-node.md Outdated Show resolved Hide resolved
Copy link
Member

@otherview otherview left a comment

Choose a reason for hiding this comment

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

Overall lgtm, I am starting to think that it can be benefitial to follow the same API struct pattern as the api package and perhaps having a couple of kickstarter unit tests ?

Comment on lines +99 to +103
func HTTPHandler(logLevel *slog.LevelVar) http.Handler {
router := mux.NewRouter()
router.HandleFunc("/admin/loglevel", logLevelHandler(logLevel))
return handlers.CompressHandler(router)
}
Copy link
Member

Choose a reason for hiding this comment

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

What if the same pattern of the api package was used ?
Maybe something like:

admin/api.go - similar to api/api.go
    The struct that handles start and stop of the server and mounts the endpoints.
    
admin/log.go - similar to accounts/accounts.go 
    The struct that has handlers for the log level changes and mounts into a subrouter
    

Copy link
Member Author

Choose a reason for hiding this comment

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

Wouldn't it make more sense to use the same pattern as the metrics server since our goal is something similar? In the metric's case the startMetricsServer is in the utils.go file while the handler is returned from the telemetry.go file similar to my admin.go file. If not are you suggesting I move startMetricsServer into its own file basically?

cmd/thor/flags.go Show resolved Hide resolved
@@ -587,6 +592,27 @@ func startMetricsServer(addr string) (string, func(), error) {
}, nil
}

func startAdminServer(addr string, logLevel *slog.LevelVar) (string, func(), error) {
Copy link
Member

Choose a reason for hiding this comment

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

Should this live in the admin package. And starting the admin server would follow the same patter as the api package?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure. For this I just used the same pattern that is used in metrics with startMetricsServer living in utils.go.

// Distributed under the GNU Lesser General Public License v3.0 software license, see the accompanying
// file LICENSE or <https://www.gnu.org/licenses/lgpl-3.0.html>

package admin
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it would be good to have some unit tests ? Since we're setting the package up, perhaps it would make any refactor easy at this stage.

otherview
otherview previously approved these changes Aug 19, 2024
leszek-vechain
leszek-vechain previously approved these changes Aug 19, 2024
@otherview otherview merged commit c00c21e into vechain:master Aug 19, 2024
12 checks passed
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.

6 participants