refactor(buck2_common): don't overload the user with HTTP retry backoff warnings

[thoughtpolice]

Summary: Another attempt at GH-316 after feedback given on GH-321 by @krallin. This introduces a new trait to dispatch these warnings, and two trait impls: one used in the materializer codepath that now writes buck events to the log, and another one used in the manifold client path to just warn to the console, like we do now.

Test Plan: Build a project that uses reindeer with buck2, as that causes a lot of warnings. Observe lots of warnings. Apply patches. Rebuild with new buck2. Observe no warnings. Run buck2 log show | grep HttpRequestRetried and see that all the new log events actually did get emitted and go where intended.

Fixes GH-316.

[thoughtpolice]

Actually, I have no idea why this isn't just a higher order function that's passed to http_retry? We don't need a trait at all?

[thoughtpolice]

Actually, I guess the trait isn't too bad, I just kind of hate the boilerplate-ness of this in particular, which we do three times here:

struct F {}

impl F {
    fn new() -> Self {
        Self {}
    }
}

But surely there's a shorter way to just make this impl one-shot on the struct declaration with a derive thingy? Rust programmers love those, I think.

I could also remove the Noop variant for the dispatch warning trait, though I'd have to rebase the history a bit, which is fine. I just wanted something to use when introducing the API patch-by-patch.

[thoughtpolice]

But surely there's a shorter way to just make this impl one-shot on the struct declaration with a derive thingy? Rust programmers love those, I think.

This is apparently the Default trait, so I can replace all these with that, I think.

[krallin]

Suggested change

	struct ManifoldDispatchableHttpRetryWarning {}
	impl ManifoldDispatchableHttpRetryWarning {
	pub fn new() -> Self {
	Self {}
	}
	}
	struct ManifoldDispatchableHttpRetryWarning;

And then just do ManifoldDispatchableHttpRetryWarning instead of ManifoldDispatchableHttpRetryWarning::new ?

[krallin]

(same as above)

[krallin]

(nit) passing &R might make a bit more sense considering that' all the API requires

[krallin]

Can we rename retries -> attempts? The first time this function is called the value will be 1 so it's more accurately attempts than retries

[krallin]

(see my comment below, it's not actually retries, it's attempts)

[krallin]

So here what we actually would like to call is this function:

pub fn broadcast_instant_event<E: Into<buck2_data::instant_event::Data> + Clone>(
    event: &E,
) -> bool {
    let mut has_subscribers = false;

    for cmd in ACTIVE_COMMANDS.lock().values() {
        cmd.dispatcher.instant_event(event.clone());
        has_subscribers = true;
    }

    has_subscribers
}

(as-is unfortunately the logging wouldn't work when using the deferred materializer)

This function lives in buck2_server, so what we need to do is allow this to be injected into the callsites.

Maybe the easiest way to do this is to simply have the HttpClient provide its logging method (because this is already threaded through to all the places you need it), via a method like:

trait HttpClient {
  ..

  fn retry_logger(&self) -> &dyn DispatchableHttpRetryWarning;
}

You probably should actually keep the "log to stderr" implementation as just the default implementation for this method on HttpClient. This way all you need to do is just wrap the client being constructed for the server with the event-logging code.

So, putting it all together, what we need to do is:

• Move this code to buck2_server
• Have it implement the broadcast
• When we create the server's http client (in DaemonState::init_data), we wrap it with some new struct implementing HttpClient that uses the event-logging implementation
• When we create ManifoldClient's http client, you inject the other one in
• When calling http_retry you get the retry logging from HttpClient

I can also make some of those changes if you'd like, just let me know

[ndmitchell]

@thoughtpolice - are you planning to follow up on this or should we close it?

[thoughtpolice]

Yes, I haven't gotten back to this, but it still is on my radar and I'd like to finish it up at some point. But if someone else gets to it or whatnot, we can close this.

[facebook-github-bot]

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.