-
Notifications
You must be signed in to change notification settings - Fork 1
HTTP client & tracing #11
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
base: master
Are you sure you want to change the base?
Conversation
Сейчас поддержка прогресса сделана через tracing-indicatif. Возможно, стоит ещё прикрутить OSC9 progress для красоты |
Я бы сказал что поддержка со стороны терминалов слишком мала чтобы думать об этом в chainql Если однако ты добавишь это в indicatif, чтобы оно делало OSC9 для суммарно всех выбранных прогрессбаров (opt-in) - то можно и использовать |
В апстриме есть пожелание это иметь: console-rs/indicatif#596 |
Состояние indicatif пустое (Ни один из spanов не заopt-inился в отображение прогресса) - делаем 0 Что касаемо error/warning - в indicatif есть возможность удалить прогрессбар с сообщением, и тогда он ещё несколько секунд висит с сообщением... Однако будто бы оно мерцать будет так, и error/warning полезны только для программ с интерактивщиной... |
.await?; | ||
|
||
if response.status() == StatusCode::TOO_MANY_REQUESTS | ||
|| response.status() == StatusCode::GATEWAY_TIMEOUT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
В случае отсутствия гейтвея - ошибка упадёт выше,
.send()
.await?;
При этом rate_limiter уведомлён не будет
Я бы сделал чот типа
struct PendingRequest<'r> {
rate_limiter: &'r RateLimiter,
succeeded: bool,
}
impl PendingRequest<'_> {
fn success(&mut self) {
self.succeeded = true;
}
}
impl Drop for PendingRequest<'r> {
fn drop(&mut self) {
// Для красоты методы можно заинлайнить, их наверное нет смысла держать в rate_limiter отдельно с таким апи
if self.succeeded {
self.rate_limiter.request_succeeded();
} else {
self.rate_limiter.request_limited();
}
}
}
impl RateLimiter {
fn pending(&self) -> PendingRequest<'_> {
PendingRequest {
rate_limiter: self,
succeeded: true,
}
}
}
И в коде применял так:
let pending = rate_limiter.pending();
let response = client.do().await?.something().await?;
if response.success {
pending.success()
}
И весь трекинг будет автоматическим
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Йуп, так было бы правильнее. Я прикручивал rate limit наспех и планировал это отрефакторить. Но забыл
|
||
impl RateLimiter { | ||
fn new() -> Self { | ||
Self(Mutex::new(RateLimiterState { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Асинхронные мьютексы тут излишни, поскольку обращение к ним сильно изолировано, и в целом они хватаются на короткий срок - было бы лучше использовать синхронные, это также позволит реализовать паттерн как я описал выше (Потому что await внутри Drop вызвать нормально нельзя)
Ничего плохого в этом нет: https://docs.rs/tokio/latest/tokio/sync/struct.Mutex.html#which-kind-of-mutex-should-you-use
If the value behind the mutex is just data, it’s usually appropriate to use a blocking mutex such as the one in the standard library
let mut this = self.0.lock().await; | ||
|
||
if let Some(last_requested_at) = this.last_requested_at.replace(requested_at) { | ||
(Duration::from_secs(60) / this.rps) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Если у тебя from_secs(60), то это не rps (requests per second), а rpm (requests per minute)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Когда то это было действительно rps. Надо будет переименовать, спасибо
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
С rps тип этого поля должен быть не u32, а f32 (Duration имеет методы для умножения/деления/сложения с f32... Кстати почему это методы, а не impl Div for Duration?.. https://doc.rust-lang.org/std/time/struct.Duration.html#method.mul_f32), потому что ситуация когда при большой нагрузке гейтвей сможет обрабатывать меньше одного запроса в секунду вполне реален. Меньше одного запроса в минуту уже менее реалистично, и наверное нет смысла такое поддерживать?..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Хотя для таких случаев тут нужен уже каскад из AIMD, где внешний AIMD будет подстраивать факторы внутреннего... Да, лучше просто rpm
And rewrite logs with tracing