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

Add initial version of the debug info gathering script #4352

Merged
merged 8 commits into from
Jul 1, 2024

Conversation

jacekn
Copy link
Contributor

@jacekn jacekn commented Jun 7, 2024

What

Add initial version of the debug info gathering script

Why

We want to make it easier to gather information useful for troubleshooting

### What

Add initial version of the debug info gathering script

### Why

We want to make it easier to gather information useful for troubleshooting
@jacekn jacekn self-assigned this Jun 7, 2024
@jacekn jacekn requested a review from SirTyson June 7, 2024 16:26
scripts/gather_debug_info Outdated Show resolved Hide resolved
@jacekn jacekn marked this pull request as ready for review June 13, 2024 12:00
@SirTyson
Copy link
Contributor

Thanks for putting this together! I think it mostly looks like we have everything we need, but there are a few extra pieces that could be useful:

  1. Optionally specify a core executable path. This isn't super important because all the production use cases that we're targeting should just be using stellar-core, but during development we often use one of builds that specify a specific path, and this script could be useful.
  2. Zip DB when using a SQLite backend. I don't think this is possible with Postgres, but I think (and could be wrong) that it's relatively easy to do in SQLite. We're in the process of getting rid of Postgres support anyway, so if you can parse the SQLite DB image path from the config and include it in the final zip result, it would be useful.
  3. Attempt to parse log path from the config file. I'm not sure what log systems other folks use, but during testing runs I just use a single file as my running log. If most everyone is encoding time info into their logs then you can skip this, I might be the odd one out here. This isn't super important so feel free to skip it in the interest of time, it might be a little tricky to detect when the config specifies a deterministic log name vs. a date based one.

* allow for custom stellar-core binary paths
* try to get logfile location from the config
* add flag to gather sqlite DB
@jacekn
Copy link
Contributor Author

jacekn commented Jun 19, 2024

Thanks for putting this together! I think it mostly looks like we have everything we need, but there are a few extra pieces that could be useful:

1. Optionally specify a core executable path. This isn't super important because all the production use cases that we're targeting should just be using `stellar-core`, but during development we often use one of builds that specify a specific path, and this script could be useful.

Done

2. Zip DB when using a SQLite backend. I don't think this is possible with Postgres, but I think (and could be wrong) that it's relatively easy to do in SQLite. We're in the process of getting rid of Postgres support anyway, so if you can parse the SQLite DB image path from the config and include it in the final zip result, it would be useful.

Done

3. Attempt to parse log path from the config file. I'm not sure what log systems other folks use, but during testing runs I just use a single file as my running log. If most everyone is encoding time info into their logs then you can skip this, I might be the odd one out here. This isn't super important so feel free to skip it in the interest of time, it might be a little tricky to detect when the config specifies a deterministic log name vs. a date based one.

Done. Logs with date in the filename are a bit tricky but for this reason the script tries to get last 24h worth of files with .log suffix. So with this this in place I believe we'll be fine.

@anupsdf
Copy link
Contributor

anupsdf commented Jun 20, 2024

Looks like the cargo-deny CI check is failing because of a new security advisory https://rustsec.org/advisories/RUSTSEC-2024-0344
We might explore putting this advisory in the ignore list since we cannot switch to curve25519-dalek 4.1.3 prior to protocol 22.

Copy link
Contributor

@SirTyson SirTyson left a comment

Choose a reason for hiding this comment

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

Seems like it's working great, thanks for working on this!

@jacekn
Copy link
Contributor Author

jacekn commented Jun 26, 2024

@anupsdf @SirTyson merge is blocked on the cargo-deny check which is unrelated to this change.
Can I leave it up to you to address the problem with the check?

@SirTyson
Copy link
Contributor

We're currently changing our Github CI so we can't merge anything at the moment, but should have it resolved in a day or two.

@SirTyson
Copy link
Contributor

CI is back online. If you rebase this off master, the last CI check should be resolved then I can merge

@jacekn
Copy link
Contributor Author

jacekn commented Jul 1, 2024

@SirTyson all checks pass now so if it looks good from our side can you merge?

@SirTyson SirTyson added this pull request to the merge queue Jul 1, 2024
Merged via the queue into stellar:master with commit 626f18a Jul 1, 2024
14 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.

4 participants