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

improve ingest lock #5296

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

improve ingest lock #5296

wants to merge 4 commits into from

Conversation

trinity-1686a
Copy link
Contributor

  • add metric for how long a lock was held
  • when a log is held for too long, log which purpose it had
  • when waiting for too long, log why we wanted to acquire the lock
  • await inside the macro to force awaiting at the right time (currently, we sometime await outside of the macro, meaning we record an instant access to the lock, and then actually proceed to acquire the lock). I'm not a fan of awaiting inside a macro as it's hidden control flow, but i consider it worth the easy-to-miss mistake

@trinity-1686a trinity-1686a requested a review from fmassot August 2, 2024 14:20
Copy link

github-actions bot commented Aug 2, 2024

On SSD:

Average search latency is 1.0x that of the reference (lower is better).
Ref run id: 3649, ref commit: 26d983d
Link

On GCS:

Average search latency is 1.01x that of the reference (lower is better).
Ref run id: 3650, ref commit: 26d983d
Link

struct TimedMutexGuard<T> {
guard: T,
acquired_at: std::time::Instant,
purpose: [&'static str; 2],
Copy link
Contributor

@fulmicoton fulmicoton Sep 18, 2024

Choose a reason for hiding this comment

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

one of the sad things with relying on dynamic labels like this, is that you end up doing one hashmap lookup very time you need to access the counter (here once per lock).

It is probably not catastrophic in this case, but here is the alternative.

have a histogram: directly on the TimedMutexGuard

histogram: &'static Histogram,

You can then cache the Histogram counter in different ways. In ingest v2 you can see the pattern done here. The value are stored in the metrics.rs directly. https://github.com/quickwit-oss/quickwit/blob/main/quickwit/quickwit-ingest/src/ingest_v2/metrics.rs#L59-L74.

You could also cache it closer to the usage site if you prefer.

I kind of like to keep the MetricsVec definition in metrics.rs, because it really helps to maintain some consistency in the naming/help etc. and it makes it easier to search for the relevant metrics name...

But it is less critical for metrics labels...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i like this pattern of statically defining things, however it's a kind of a pain, and a large typo hazard to implement. Having some sort of macro to derive "Build a static (histogram|gauge|counter) vec from a dynamic vec" would be nice, and would likely make us use this more

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.

2 participants