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

Add --debug=rpc and --debug=profiling flags for enabling debug output #3352

Merged
merged 12 commits into from
May 23, 2023
Merged

Add --debug=rpc and --debug=profiling flags for enabling debug output #3352

merged 12 commits into from
May 23, 2023

Conversation

romac
Copy link
Member

@romac romac commented May 22, 2023 •
edited
Loading

Closes: #2852

Description

This PR implements the proposition in #2852 and removes the profiling feature flag.

PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests: integration (for Hermes) or unit/mock tests (for modules).
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).
  • Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

@romac romac requested a review from ljoss17 May 22, 2023 11:35
@ljoss17
Copy link
Contributor

ljoss17 commented May 22, 2023

Since we remove the profiling feature, won't we see this message for each time! call https://github.com/informalsystems/hermes/pull/3352/files#diff-9d723ef7a1f09059ad4272facc9d5ea2b31ecb46aadadfcf8554c3933a5ca52dR38 ?

@romac
Copy link
Member Author

romac commented May 22, 2023 •
edited
Loading

Good catch! Fixed in f61aa8a

ljoss17
ljoss17 reviewed May 22, 2023
View reviewed changes
crates/relayer/src/util/profiling.rs

impl Drop for Timer {
fn drop(&mut self) {
let enabled = enabled();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let enabled = enabled();
if !enabled() {
return;
}
let enabled = enabled();

What about skipping everything if profiling isn't enabled, do you see any downside to this ?

Copy link
Member Author

@romac romac May 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I would like to give users the option to use either console or JSON profiling, but we can still skip if none is enabled, good idea!
bc105a8

@romac romac marked this pull request as ready for review May 23, 2023 08:29
@romac
Copy link
Member Author

romac commented May 23, 2023

Still needs a changelog entry

@ljoss17
Copy link
Contributor

ljoss17 commented May 23, 2023

Do we want to enable profiling-json for CLIs which are not hermes start? If so, we need to set the file when we update the debug_sections flag

@romac
Copy link
Member Author

romac commented May 23, 2023 •
edited
Loading

Do we want to enable profiling-json for CLIs which are not hermes start? If so, we need to set the file when we update the debug_sections flag

I don't think it's needed, the console profiling should work fine for one off commands

@romac
Copy link
Member Author

romac commented May 23, 2023 •
edited
Loading

Or perhaps only for clear packets, but then we may as well add support everywhere. Which I think can be done in the CliApp directly.

@ljoss17
Copy link
Contributor

ljoss17 commented May 23, 2023

Or perhaps only for clear packets, but then we may as well add support everywhere. Which I think can be done in the CliApp directly.

Yes, I was wondering if we should add it here https://github.com/informalsystems/hermes/pull/3352/files#diff-f4ab9a0749ae805544d3bd5157591c8888bd1ae933688a71b63633d5fd791b94R197.
But if there is only one CLI which could potentially use this, it might be better to document in the guide that this debug option is for a hermes start instance.
I'll add the changelog and update the guide.

ljoss17
ljoss17 approved these changes May 23, 2023
View reviewed changes
Copy link
Contributor

@ljoss17 ljoss17 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. Tested it and seems to work fine.

@romac
Update changelog entry
fbb5f84 
Signed-off-by: Romain Ruetschi <romain.ruetschi@gmail.com>
@romac romac merged commit 7d74815 into master May 23, 2023
@romac romac deleted the romac/debug-sections branch May 23, 2023 13:55
pratikbin pushed a commit to pratikbin/hermes that referenced this pull request May 24, 2023
@romac @ljoss17
Add --debug=rpc and --debug=profiling flags for enabling debug ou…
83a270a 
…tput (informalsystems#3352)

* Add `--debug=rpc` flag for enabling Tendermint RPC debug info

* Add `--debug=rpc` flag for enabling profiling output

* Only output profiling info on console if profiling is enabled

* Allow comma-separated values

* Re-generate templates

* Add `json-profiling` debug section, change env variable to `PROFILING_DIR`

* Add newline after each JSON entry

This requires a mutex around the File, otherwise we may end up
with garbled entries when two threads write the file concurrently.

* Use atomics instead of once cell for profiling flags

* Avoid doing any work when profiling is disabled

* Update templates

* Add note to guide for profiling-json. Add changelog entry

* Update changelog entry

Signed-off-by: Romain Ruetschi <romain.ruetschi@gmail.com>

---------

Signed-off-by: Romain Ruetschi <romain.ruetschi@gmail.com>
Co-authored-by: Luca Joss <luca@informal.systems>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljoss17 ljoss17 ljoss17 approved these changes

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

Successfully merging this pull request may close these issues.

Add --debug global flag
2 participants
@romac @ljoss17