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

refactor: add config struct and use slog #354

Merged
merged 1 commit into from
Mar 21, 2024
Merged

Conversation

gligneul
Copy link
Contributor

  • Move node to internal package
  • Add a struct that contains all node config
  • Add structured logging with slog
  • Add CARTESI_LOG_PRETTY
  • Remove CARTESI_LOG_TIMESTAMP
  • Log Rust service's messages in the correct level
  • Print cartesi-rollups-cli to stdout

Co-Author: Victor Yves Crispim [email protected]
Co-Author: Francisco Moura [email protected]

@gligneul gligneul requested review from fmoura and torives March 14, 2024 19:09
@gligneul gligneul force-pushed the feature/refactor-config branch 2 times, most recently from aff7ff9 to d2acc48 Compare March 14, 2024 20:08
@gligneul gligneul linked an issue Mar 14, 2024 that may be closed by this pull request
@gligneul gligneul added this to the 1.4.0 milestone Mar 14, 2024
@gligneul gligneul added the #feat:go-supervisor Feature: Go supervisor label Mar 14, 2024
@gligneul gligneul self-assigned this Mar 14, 2024
@gligneul gligneul marked this pull request as ready for review March 14, 2024 20:22
@gligneul gligneul requested a review from renan061 March 14, 2024 20:22
fmoura
fmoura previously approved these changes Mar 15, 2024
@endersonmaia
Copy link
Contributor

While testing the logs, I see some \ escaping and I don't think they're needed, right?

validator-1  | time=2024-03-15T11:08:45.413Z level=INFO msg="INFO dispatcher::machine::rollups_broker: producing input event event=RollupsInput { parent_id: \"0\", epoch_index: 0, inputs_sent_count: 1, data: AdvanceStateInput(RollupsAdvanceStateInput { metadata: InputMetadata { msg_sender: f39fd6e51aad88f6f4ce6ab8827279cfffb92266, block_number: 13, timestamp: 1710500920, epoch_index: 0, input_index: 0 }, payload: 3q2+7w==, tx_hash: 71932220268ade67936c208a119841cf9ef5b8b3b36ab380740fd814401baf2f }) }" service=dispatcher

This when sending an input:

validator-1  | time=2024-03-15T11:08:45.426Z level=INFO msg="\x1b[0m\x1b[38;5;8m[\x1b[0m\x1b[0m\x1b[32mINFO \x1b[0m rollup_http_server::http_service\x1b[0m\x1b[38;5;8m]\x1b[0m Received new request of type ADVANCE" service=server-manager

@gligneul
Copy link
Contributor Author

@endersonmaia they are added automatically because they are already inside another string. msg="INFO...

@endersonmaia
Copy link
Contributor

@endersonmaia they are added automatically because they are already inside another string. msg="INFO...

Yeah, they're useless and are extra bytes in the log stream. Is it hard to avoid/remove them?

And what about the second example?

msg="\x1b[0m\x1b[38;5;8m[\x1b[0m\x1b[0m\x1b[32mINFO \x1b[0m

Looks like terminal coloring from the inner rust service logs.

Copy link
Contributor

@torives torives left a comment

Choose a reason for hiding this comment

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

Oh boy, this is looking neat! I loved how text/template made things so much easier to understand. I've only left a request concerning the logs and other small things.

docs/config.md Outdated Show resolved Hide resolved
cmd/cartesi-rollups-node/main.go Outdated Show resolved Hide resolved
internal/node/config/config.go Outdated Show resolved Hide resolved
torives
torives previously approved these changes Mar 20, 2024
Copy link
Contributor

@torives torives left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

- Move node to internal package
- Add a struct that contains all node config
- Add structured logging with slog
- Add CARTESI_LOG_PRETTY
- Remove CARTESI_LOG_TIMESTAMP
- Log Rust service's messages in the correct level
- Print cartesi-rollups-cli to stdout

Co-Author: Victor Yves Crispim <[email protected]>
Co-Author: Francisco Moura <[email protected]>
@gligneul gligneul merged commit a728248 into main Mar 21, 2024
6 checks passed
@gligneul gligneul deleted the feature/refactor-config branch March 21, 2024 12:56
@torives torives mentioned this pull request Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
#feat:go-supervisor Feature: Go supervisor
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Refactor config package
4 participants