Conversation
c-cube
commented
Apr 21, 2026
c-cube
commented
- modifies how loggers work so it's easier to swap them at runtime
- pass timestamp and a list of key/value pairs
- normal callsites for logging are unaffected, but it enables us to have richer logging.
c-cube
force-pushed
the
sc/structured-logging
branch
from
c2eced1 to
07ed94a
Compare
c-cube
force-pushed
the
sc/structured-logging
branch
from
de9c75f to
ba7e6f4
Compare
the goal is to preserve the current behavior, but enable: - alternative formats (logfmt, later ndjson/jsonl) - runtime swap of log format, no functor
easier on angle-grinder and on the eyes in general
c-cube
force-pushed
the
sc/structured-logging
branch
from
ba7e6f4 to
c87470a
Compare
|
GHA/ocaml-setup is doing weird stuff, but otherwise it's ready for review. |
|
will merge wed 5/6 |
rr0gi
left a comment
rr0gi
left a comment
There was a problem hiding this comment.
i don't like that it changes the format of default logging
i would suggest having another parameter that keeps old message in default logfmt and uses pairs in rich (in addition to current ~pairs which i understand appends pairs to log message always)
| let do_fork () = | ||
| match check_req req with | ||
| | `Error err -> Exn.fail "pre fork %s : %s" (show_request req) (Unix.error_message err) | ||
| | `Error err -> Exn.fail "pre fork : %s" (show_request req) (Unix.error_message err) |
| let verbose_curl_result nr_http action t h code = | ||
| let open Curl in | ||
| let b = Buffer.create 10 in | ||
| bprintf b "%s #%d %s ⇓%s ⇑%s %s " |
There was a problem hiding this comment.
so how will Web request log look like now?
|
Ok, I'll keep the old code and switch between the two based on the current log format, to get 100% compatibility with existing logs.
|
|
Ok, the web and http logs should be back to (almost) exactly what they were, modulo a bit of punctuation (some ":" added). The logging is more uniform now, and checks what logging backend we use to log accordingly. |
c-cube
force-pushed
the
sc/structured-logging
branch
from
06db0f7 to
0a08364
Compare
| let debug_s = State.logger.debug_s | ||
| let info_s = State.logger.info_s | ||
| let warn_s = State.logger.warn_s | ||
| let error_s = State.logger.error_s | ||
| let critical_s = State.logger.critical_s | ||
| let put_s = State.logger.put_s | ||
| let debug = State.logger.debug | ||
| let info = State.logger.info | ||
| let warn = State.logger.warn | ||
| let error = State.logger.error | ||
| let critical = State.logger.critical |
There was a problem hiding this comment.
no need to export these here, they are overridden below with make_s
There was a problem hiding this comment.
the ones below are only visible within the class, they're not at toplevel. But I could remove debug_s, info_s, etc as they might not be super useful?
| in { put } | ||
|
|
||
| (** A logger *) | ||
| type t = { |
There was a problem hiding this comment.
but also do we even need this 🤔
single put_s function would be "easier" for Log.log to build upon
There was a problem hiding this comment.
so just use the type put above? can do
2 participants
