Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a -tracing flag that generates debugging/tracing calls #117

Closed
wants to merge 3 commits into from

Conversation

ELLIOTTCABLE
Copy link

The output will generally differ in a call to T.tracing_span ~operation ~tables ~statement name @@ fun () -> …, something like this:

index 661f0dd72f7..506d44aeca3 100644
--- a/db_gen/sth.ml
+++ b/db_gen/sth.ml
@@ -2,7 +2,7 @@
 (*  *)
 (* generated by sqlgg *)
 
-module Make (T : Sqlgg_traits.M_io) = struct
+module Make (T : Sqlgg_traits.M_tracing_io) = struct
 
   module IO = T.IO
 
@@ -20,6 +20,7 @@ WHERE user_id = ?")
       T.set_param_Int p user_id;
       T.finish_params p
     in
+    T.tracing_span ~operation:"SELECT" ~statement:__sqlgg_sql "some_procedure" @@ fun () ->
     T.select db __sqlgg_sql set_params invoke_callback
 
   module Fold = struct
@@ -38,6 +39,7 @@ WHERE user_id = ?")
         T.finish_params p
       in
       let r_acc = ref acc in
+      T.tracing_span ~operation:"SELECT" ~statement:__sqlgg_sql "some_procedure" @@ fun () ->
       IO.(>>=) (T.select db __sqlgg_sql set_params (fun x -> r_acc := invoke_callback x !r_acc))
       (fun () -> IO.return !r_acc)
  ...

@ELLIOTTCABLE ELLIOTTCABLE marked this pull request as ready for review September 29, 2023 00:49
@rr0gi
Copy link

rr0gi commented Nov 9, 2023

Isn't it possible to do this in the implementation of the traits? Can add extra arguments as nedeed

@ELLIOTTCABLE
Copy link
Author

ELLIOTTCABLE commented Nov 14, 2023

Theoretically, yes; but I'd argue that it's inherent to tracing that you …

  1. 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.

  2. 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!)

  3. want tracing events to have as much metadata as possible; and

  4. 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

@rr0gi
Copy link

rr0gi commented Nov 15, 2023

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.

@rr0gi
Copy link

rr0gi commented Nov 15, 2023

also current approach requires changes to build system to enable tracing, but in traits it will be available on upgrade to everybody automatically

@ELLIOTTCABLE
Copy link
Author

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?

@ygrek
Copy link
Owner

ygrek commented Feb 1, 2024

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

@ygrek ygrek closed this Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants