Skip to content

emit warning if bl merge slower than 50 seconds#4345

Closed
ThomasBrady wants to merge 1 commit intostellar:masterfrom
ThomasBrady:275-emit-log-bl-merge-50-sec
Closed

emit warning if bl merge slower than 50 seconds#4345
ThomasBrady wants to merge 1 commit intostellar:masterfrom
ThomasBrady:275-emit-log-bl-merge-50-sec

Conversation

@ThomasBrady
Copy link

@ThomasBrady ThomasBrady commented Jun 4, 2024

Description

Resolves https://github.com/stellar/stellar-core-internal/issues/275

Checklist

  • Reviewed the contributing document
  • Rebased on top of master (no merge commits)
  • Ran clang-format v8.0.0 (via make format or the Visual Studio extension)
  • Compiles
  • Ran all tests
  • If change impacts performance, include supporting evidence per the performance document

@ThomasBrady ThomasBrady force-pushed the 275-emit-log-bl-merge-50-sec branch 2 times, most recently from a484e88 to 74b26a6 Compare June 4, 2024 18:33
doubleTime.count() / availableTime.count() * 100;
auto timeSec =
std::chrono::duration_cast<std::chrono::seconds>(time);
if (time > std::chrono::seconds(50))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use LogSlowExecution with threshold of 50 seconds for this?

Copy link
Author

Choose a reason for hiding this comment

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

I considered that, but we currently have (and I assume want to keep?) a debug log for every merge. If I change to use LogSlowExecution with a threshold of 50, then we will get a debug log for merges at the threshold and a warn (10x threshold) log for merges at 500 seconds. I could set a threshold of 5 seconds for a debug at 5 and a warn at 50. The debug would be duplicated in that case but I guess its not a big deal.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I think LogSlowExecution with a threshold of 25 is a good middle ground, which gives us debug at 25 and warning at 250 right? BucketListDB is pretty mature so idk if we need super granular debug logs these days.

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry what i meant is that we should only warn if bucket merge crosses 1 min mark (so adjust the threshold accordingly). we should still debug log for all merges though.

Copy link
Contributor

@SirTyson SirTyson Jul 19, 2024

Choose a reason for hiding this comment

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

Sure, I might actually up that though to 2 minutes. 1 minute is pretty standard for big buckets and doesn't indicate that anything is wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah that seems fine

…gSlowExecution for bucket merges longer than 2 minutes
@ThomasBrady ThomasBrady force-pushed the 275-emit-log-bl-merge-50-sec branch from 74b26a6 to 0829a25 Compare August 14, 2024 21: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