feat(database): add QueryExecuted event with query observability

[xHeaven]

Adds database observability via events.

Heavily WIP, any comments are welcome.

[xHeaven]

cc @brendt @innocenzi because GitHub won't request review until the PR is ready. Do you think this is a good approach?

[innocenzi]

I'm curious to understand why the event dispatcher needed being wrapped and what's the purpose of the unset in it?

Also not a fan of the dependency location in QueryExecuted (and you'd need to pass the correct database tag anyway), I think all the debugging in there is a biiit overkill, I'd prefer deferring its logic elsewhere, kind of like the RawSql class.

Otherwise, yes, we definitely want good observability (and that's something I already touched on on my local debug UI branch)!

[xHeaven]

I'm curious to understand why the event dispatcher needed being wrapped and what's the purpose of the unset in it?

The dispatcher wraps EventBus so if the listener throws for whatever reason, it doesn't break the actual query flow. Since we are dispatching in a finally block, any exception would mask the real query result (or a possible QueryWasInvalid exception).
The unset is nothing functional, it's just there so Mago doesn't harass me with linter warnings for empty catch blocks, and I don't have to suppress it with that ugly mago-expect thing.

Also not a fan of the dependency location in QueryExecuted (and you'd need to pass the correct database tag anyway)

This one is fair, definitely something that needs a bit more thinking.

I think all the debugging in there is a biiit overkill, I'd prefer deferring its logic elsewhere, kind of like the RawSql class.

This makes sense too, a separate utility class would likely be better.

Otherwise, yes, we definitely want good observability (and that's something I already touched on on my local debug UI branch)!

I'm very curious to see that local debug UI branch 👀 Should I continue exploring things here considering you've already started something (probably) similar?

[innocenzi]

Should I continue exploring things here considering you've already started something (probably) similar?

Definitely do!

The unset is nothing functional, it's just there so Mago doesn't harass me with linter warnings for empty catch blocks, and I don't have to suppress it with that ugly mago-expect thing.

I get it, but let's use @mago-ignore still 🙏 I prefer that comment over code that looks like some weird magic trick

That being said, since the event dispatcher is used in only one place, I'd rather not abstract it in a class. We'd need to have an interface instead (so it could be replaced), and the naming is confusing too. If we were to keep it, I'd rather have a database-specific implementation of EventBus (we even have decorator support so you could use that).

But in my opinion, a try/catch inside the finally is enough?

[xHeaven]

I get it, but let's use @mago-ignore still 🙏 I prefer that comment over code that looks like some weird magic trick

I've done that in a previous commit 👀

That being said, since the event dispatcher is used in only one place, I'd rather not abstract it in a class. We'd need to have an interface instead (so it could be replaced), and the naming is confusing too. If we were to keep it, I'd rather have a database-specific implementation of EventBus (we even have decorator support so you could use that).

But in my opinion, a try/catch inside the finally is enough?

I kind of agree, it actually started that way, but it felt too much. I'll bring it back, we'll see.

---

Done with the edits, decided to remove the extra class in favor of the try-catch. Not sure if I want to stretch the scope of this PR, and if so, how much.

Do we like this as-is? What do we not like? What should I add?