Exposing dropped log messages

Hello!

I see that Suricata could potentially drop logs (eg., SCLogFileWriteSocket) and (I believe!) this is only exposed when Suricata stops (it’ll log a line per logger ctx which contains drops).

I think this would be beneficial to expose in the suricata stats log file (broken down per logger type).
eg. a “dropped.flow.json” stat which in incremented whenever the logger attached to flow.json isn’t able to write a log.

Seeing as I’m still relatively new to this code base, I’m looking for advice on how to accomplish this (or input on whether you believe this is reasonable).

Allocating and incrementing the counter requires access to ThreadVars, which is much higher up the stack (eg. at the JsonFlowLogger() level, rather than the socket writing layer).

I figure I have a few different options…

  1. Expose ThreadVars all the way down the stack to where the socket writes (this method already accepts a thread ID, but it doesn’t otherwise need the full ThreadVars structure, so this is wasteful from that perspective, but not much of a change perf wise, I think… passing a pointer rather than an integer). In does introduce dependencies which may cause issues, though, no sure.

  2. Expose the error up the stack. There’s already return codes for many of these functions (eg. OutputJsonBuilderBuffer(), SCLogFileWrite(), return ints, but they always return 0), so those functions would have to be modified to return actual success/fail details. It’s the layer above OutputJsonBuilderBuffer() which contains ThreadVars access, however, so the change to increment the stat would be replicated in many different places (output-json-*.c, et al) which seems less ideal.

  3. Something in between; expose the error up to the OutputJsonBuilderBuffer() level, and expose the ThreadVars down into OutputJsonBuilderBuffer() and have that method perform the increment of the stat. I’d still have to modify every place that calls OutputJsonBuilderBuffer() but only to pass in the ThreadVars as well.

What’re you thoughts on the above approaches?
Am I missing another more obvious approach?
Which of these would be preferred?

Thanks,
Jeff

One thing I’ve been considering is removing the ThreadVars from all the calls and instead make it a thread_local “global”. I think since Suricata 6 we require thread local support (either through the c11 support or gnu99’s __thread support), so it should be safe. Just wondering if there would be side effects I’m overlooking.

Ahh; okay, excellent. I was actually wondering if there was a reason for not already doing this. I can look into this angle.