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

Create a dynamic logger (Fixes #1181) #2245

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

derrix060
Copy link
Contributor

See commit messages for more information

utils/log.go Fixed Show fixed Hide fixed
Copy link

codecov bot commented Nov 4, 2024

Codecov Report

Attention: Patch coverage is 68.18182% with 28 lines in your changes missing coverage. Please review.

Project coverage is 75.45%. Comparing base (e4240ac) to head (55eb191).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
node/node.go 65.38% 7 Missing and 2 partials ⚠️
node/http.go 0.00% 7 Missing ⚠️
cmd/juno/juno.go 33.33% 6 Missing ⚠️
utils/log.go 86.36% 5 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2245      +/-   ##
==========================================
+ Coverage   75.25%   75.45%   +0.20%     
==========================================
  Files         106      106              
  Lines       11229    11266      +37     
==========================================
+ Hits         8450     8501      +51     
+ Misses       2138     2130       -8     
+ Partials      641      635       -6     

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

Ignored log and state files generated during Juno debugging in VS Code.
Prevents clutter and accidental commits of temporary debug files.
@derrix060 derrix060 force-pushed the mario/dynamic-logger branch 2 times, most recently from 4c53ba2 to 911e66a Compare November 5, 2024 22:14
Introduce HTTP server to dynamically adjust log levels at runtime.
Refactor log level handling to use a new LogLevel struct.
Update tests and documentation to reflect these changes.

Enhances logging flexibility and improves runtime configurability.
@@ -123,6 +125,7 @@ const (
defaultCorsEnable = false
defaultVersionedConstantsFile = ""
defaultPluginPath = ""
defaultLogPort = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about setting it e.g. to 9091 by default instead of random? I guess it would keep things more predictable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

0 doesn't mean random here, it acutally means that it's disabled :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -84,6 +84,8 @@ const (
corsEnableF = "rpc-cors-enable"
versionedConstantsFileF = "versioned-constants-file"
pluginPathF = "plugin-path"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add the logF flag to keep it consistent with the rest of the configuration? Like metricsF, grpcF, or httpF?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wdym? I've added this logHostF and logPortF

Do you mean a boolean flag to enable/disable the log host?

Copy link
Contributor

@pnowosie pnowosie left a comment

Choose a reason for hiding this comment

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

Nice one 👍

@derrix060 derrix060 enabled auto-merge (rebase) November 26, 2024 19:28
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.

3 participants