Closed ELLIOTTCABLE closed 9 months ago
Isn't it possible to do this in the implementation of the traits? Can add extra arguments as nedeed
Theoretically, yes; but I'd argue that it's inherent to tracing that you …
want tracing events to be pervasive (i.e. make it hard to forget to add them.)
In this case, that means adding them to the trait-implementation would make it easy for someone not paying attention to write a trait-implementation that doesn't implement the tracing; whereas "lowering" it into the sqlgg core ensures it's never forgotten.
want tracing events to be created transparently (i.e. without developer-intervention) wherever possible
Mostly, it's best to make at least some of the tracing (HTTP, SQL, the "general" topics) as invisible as possible to the everyday developer; they shouldn't have to think about it, be responsible for maintaining its presence, or be to blame if it's missing. (They have other things to be thinking about!)
want tracing events to have as much metadata as possible; and
want them created with the least abstraction, closest to the event, as possible (i.e. not several function calls / stackframes / events "away", if possible)
In the case of an SQL query, for instance, then, within the SQL library, directly around the actual networking calls, would be ideal for "exit" events like these … except that that contradicts №.3; thus it's best to "lift"" them one level up to-the-thing calling the SQL library, i.e. sqlgg's generated code.
Anyway, I'm happy to change it, if you feel strongly about it; but I really do think this is the correct approach specifically for tracing, because tracing is something of a special case.
Thoughts?
† as in: everyone, because nobody wants to think about tracing, which we don't want them worrying about anyway
One has to use traits module to use generated code, and sqlgg provides builtin ones, so can add tracing there without changing the generator and they will still be available to the user by default. If you structure it as a common module that is included by all the traits then it can be also included by in-house traits too.
also current approach requires changes to build system to enable tracing, but in traits it will be available on upgrade to everybody automatically
I've started on this in #121; but I'm not a big fan of that approach, after taking a stab at it.
The biggest issue is that this approach forces a break in backwards compatibility — and not just in theory; since now there's new, non-disable-able changes to the traits module(s) shared among the Ahrefs codebase, that approach kinda forces us to regenerate all the sqlgg-generated code. The existing approach in this PR is more backwards-compatible (both for external users, and ourselves) — we can, for example, just turn on the -tracing
flag for the build in AA.
I can probably work around the downsides in #121, but yeah, I don't really see "one line of change to build-system commands/Make" as a poor trade-off to "an additional module and potentially some functorization in OCaml" … eh.
I'll leave it up to you, though, @ygrek, as to which you prefer?
There is no such promise to never break custom traits. I think I prefer the #121 approach which is automatic for ppl without custom traits and thus helps to enable tracing transparently in more places
The output will generally differ in a call to
T.tracing_span ~operation ~tables ~statement name @@ fun () -> …
, something like this: