Add a traceable Diesel connection structure

[smklein]

FYI @bnaecker - this might be an alternative to the issues we had in oxidecomputer/async-bb8-diesel#7.

I believe all the "Span" fundamentals mentioned in that PR are still relevant, but this attempts to expose the "raw DB interface to Diesel" a little more explicitly.

[smklein]

So I went ahead and added some "print" statements here, just as a quick check before actually bothering to put probes anywhere.

Looks like "diesel::Connection::load" is getting hit for just about every single invocation here. I still think we should also instrument the "execute" functions, but load seems like the big one to inspect.

I left my print statement in, as they currently show how to extract the outgoing query to a string, but we should definitely replace them in the probed world.

[smklein]

I asked about this on the Diesel gitter, and got the following response from Georg (Diesel maintainer):

@smklein I think a custom connection implementation is the right level of abstraction to record such things. Basically any query that is executed goes through Connection (or SimpleConnection for migrations) internally. See https://crates.io/crates/diesel_logger and https://crates.io/crates/diesel-tracing for existing solutions.

(welp, that's lucky! didn't know those existed before throwing this PR here)

That written I should also note that logging statements on each call will likely have a impact on performance, as this forces the query builder to run, even if the statement is already in the prepared statement cache.
Additionally having some logging/instrumenting connection implementation in diesel itself, is definitively a feature we are interested in. As we don't really have a idea how to implement that in a generic way yet, we are interested in proposed designs there. So if you have some idea just fill a new discussion thread :)

I suspect a judicious use of "only doing work in probe closures" can help avoid this - but good to know that they're interested in feedback.

[bnaecker]

@smklein Thanks for putting this together, it's definitely encouraging. For some context, is this intended to model how we might extend async-bb8-diesel to support tracing? As in, we would implement a new AsyncConnection type (different from the trait) with these internals? Or is the intention to have this type here in Nexus?

[smklein]

@smklein Thanks for putting this together, it's definitely encouraging. For some context, is this intended to model how we might extend async-bb8-diesel to support tracing? As in, we would implement a new AsyncConnection type (different from the trait) with these internals? Or is the intention to have this type here in Nexus?

Thanks to the Joy of Generics, this is actually somewhat decoupled from async-bb8-diesel. The async "connection manager" in that crate wraps a connection type that implements R2D2Connection - as you can see in the examples, that can be PgConnection for the "concrete connection to a Postgres DB":

https://github.com/oxidecomputer/async-bb8-diesel/blob/22c26ef840075a57b18ac2eae3ead989b992773e/src/connection_manager.rs#L31

... Or it can be this new TracedPgConnection type, which also implements R2D2Connection.

So, TL;DR, I don't think async-bb8-diesel needs to be updated for this to work.

Now, the question of "where should this thing live", IMO, it could go in one of two places:

• It could live right here in Nexus, as it does currently in this PR. I think the TracedPgConnection structure should only depend on Diesel + whatever utilities we need for tracing.
  • Pros: Easier to update. Easier to add different types of tracing / logging.
  • Cons: More code in Omicron. Risk of adding dependencies on things it shouldn't depend on. Risk of duplication if we add new DB users that want tracing (outside of Omicron).
• It could be extracted it into a separate crate. This is basically what the two other Diesel logging crates I mentioned do - if our tracing code is isolated from Nexus, we could plausibly build a "Dtrace'd Diesel Connection" crate. As mentioned earlier, this would simply provide a new "connection type" that implements the necessary traits to be plugged into async-bb8-diesel - the traced connection wouldn't need to depend on the async stuff, and the async stuff wouldn't need to depend on the traced connection.
  • Pros: It's decoupled from Omicron. Potentially easier to understand in isolation. Could be used elsewhere.
  • Cons: It's another repo to depend on / update.

[bnaecker]

@smklein Thanks for that context, definitely helpful. I'd be inclined to put this into a new crate. Yes, it's a new dependency, but I think the ability to use it in other contexts might be helpful. But I'd love to hear your thoughts.

[bnaecker]

@smklein I'm finally circling back to this. I started to implement this in a standalone crate that we can push on crates.io, but it seems we're actually using a bleeding-edge version of diesel in omicron. I assume that's intentional and required, but it might mean it'd be easier or more appropriate to keep the traced connection in this repo. WDYT?

[smklein]

@smklein I'm finally circling back to this. I started to implement this in a standalone crate that we can push on crates.io, but it seems we're actually using a bleeding-edge version of diesel in omicron. I assume that's intentional and required, but it might mean it'd be easier or more appropriate to keep the traced connection in this repo. WDYT?

Yeah, we use Diesel 2, which hasn't been released, so I depend on the github version. This prevents publishing to crates.io, but doesn't necessarily mean we can't split it out. As an example, async-bb8-diesel depends on the unreleased version of Diesel, but exists as a separate crate (just unpublished).

Either way is fine with me though. Easy to split out later if that's preferable.

[bnaecker]

@smklein Sounds good, I'll make it a separate crate on GH, a la async-bb8-diesel.

[bnaecker]

I'm closing this as we've now merged #488 based off this.