Re-export `slog`

[alilleybrinker]

In general I think it's a good practice to re-export types from dependencies that are part of a crate's public API. Currently, slog::Logger is part of the public API for ConfigLogging::to_logger and ServerBuilder::new, but is not re-exported from slog. So as a user of dropshot, if I want to make a function that wraps (in my case) ConfigLogging::to_logger, I have to specify slog as a direct dependency of my crate alongside dropshot and ensure I manually keep the versions of <my_crate> > slog synced with <my_crate> > dropshot > slog. It's not impossible, but it is tedious.

It looks like dropshot currently re-exports one dependency: semver. Could the same be done for slog?

Separate, but related: when re-exporting multiple dependent crates, you may want to put them in a module together. I've seen it called deps elsewhere. Your call!

Also, thanks for building dropshot! I'm really enjoying using it so far!

[davepacheco]

Hm. I don't think this would be bad. But it seems kind of untenable for each crate that uses slog to export its own slog. The idea is to thread a common logger from the top-level through various dependencies. That can't work if they're not using the same slog.

Tangentially: I've long thought about putting the ConfigLogging (and related LogContext stuff from test_util) into a separate crate because we use that in a bunch of places that we don't otherwise need Dropshot. That wouldn't help that much here though because we're still going to wind up with ServerBuilder accepting a logger.

[hawkw]

Hm. I don't think this would be bad. But it seems kind of untenable for each crate that uses slog to export its own slog. The idea is to thread a common logger from the top-level through various dependencies. That can't work if they're not using the same slog.

If I understand correctly, I think the goal here is to make it easier to ensure that everyone is using the same slog crate version. The idea is that rather than having an explicit dependency on slog in the top-level application's Cargo.toml, which must be the same version of slog as the one that dropshot depends on (and which breaks the build if, say, renovate/dependabot updates the app's slog dependency), one would not depend explicitly on slog in the app's Cargo.toml and would instead get slog from use dropshot::slog::..., so that one does not have to independently manage a slog version.

If one has a bunch of different dependencies which all use slog, this may not get you anything, since you may still run into incompatible slog versions if a different dependency wants a different slog, but if one's only dependency which has slog in its API is dropshot, this should make it easier.

[davepacheco]

If one has a bunch of different dependencies which all use slog, this may not get you anything, since you may still run into incompatible slog versions if a different dependency wants a different slog, but if one's only dependency which has slog in its API is dropshot, this should make it easier.

This is the case I'm thinking of (since it's our actual case) and I think it's actually much worse, right? Say we use slog from dropshot, steno, qorb, etc. Would these all export their own slog? Wouldn't that be much harder to manage because you'd have to keep all of them updated in lockstep with each other and with slog? By comparison, looser Cargo.toml constraints (like "^2.7") would allow the app to easily pick one and move forward whenever all of its dependencies move forward. Sorry if I'm misunderstanding how this would work.

[alilleybrinker]

First, I'll affirm that my case is also one where only dropshot is using slog.

Second, some local exploration of the re-export vs. peer-dependency trade-off has led to discovering that for slog, just slog::Logger would be insufficient as a user anyway, since slog effectively requires additional crates to actually construct a slog::Logger (slog-term, slog-async, et al.).

Also, rereading the old Rust API Guidelines discussion on re-exports of public dependencies, I noted a comment from Ashley Mannix (KodrAus) that makes clear re-exports can be, uh, complicated much like how slog::Logger is complicated:

Personally, I think creating public dependencies by re-exporting foreign items should be at least mildly discouraged since in my experience it creates a future maintenance burden that outweighs the convenience of the re-export in the first place. As an example, I recently had to rust-cli/env_logger#75 which turned out to be a fairly complex change because the API I was effectively re-exporting was much larger than just the items I re-exported (because of trait impls).

So perhaps the peer dependency route required today is the best option for slog and dropshot.

I'll leave it up to you all whether to close the issue, but I'm satisfied that the public dependency route makes sense here, so I'll proceed in my own use of dropshot by just specifying a compatible slog version and making sure I track dropshot (at least, when incompatibility arises, things will loudly stop compiling, so that's good).

---

Relatedly (though I don't think it's dispositive in this situation):

I think part of the awkwardness here is that (as an outsider) it seems there's some tension in Dropshot about whether it's a library or a binary. By this I mean that, while the dropshot crate is obviously a library crate, it makes some design choices that feel "binary-like." For example that it selects slog as not just the interface for producing log messages (essentially as a log replacement), but as a log consumer which is globally registered (roughly an env_logger replacement, or similar) and which actively writes logs (either to stdout or to a file).

[davepacheco]

Thanks for that.

For example that it selects slog as not just the interface for producing log messages (essentially as a log replacement), but as a log consumer which is globally registered (roughly an env_logger replacement, or similar) and which actively writes logs (either to stdout or to a file).

I don't follow this but I'm interested to understand it better. Can you say more? The Dropshot server only accepts a slog::Logger as "the interface for producing log messages". I don't believe there's anything global about it, and you can construct that logger however you want, and it can write to stdout, a file, anything else you want, or nothing at all. That's all in how you set up the Logger.

Are you talking about ConfigLogging and how that presents as "the configuration for logging for the whole program"? I agree that probably doesn't belong in Dropshot (that's what I meant in this comment). But I believe nothing else in Dropshot uses it or depends on it. As a dropshot consumer, you can ignore it if you want.

[alilleybrinker]

Yeah, I did mean the ConfigLogger API, though you're right that it's not an actual requirement. I think that the dropshot docs (specifically the repo README) could be a bit clearer on this topic as well. They're written with the expectation that you'll do TOML configuration with some specific keys, which I take it is the norm within Oxide, but which (as the ConfigLogging API docs note) is not an actual requirement in the code and is not part (as far as I can tell) part of the dropshot API itself.

[davepacheco]

Thanks. That's helpful!