Sequence logging #10
Sequence logging #10
Conversation
miDeb
force-pushed
the
sequence-logging
branch
3 times, most recently
from
a22e30d to
59f0e21
Compare
|
Tested it on the server and it worked like this. |
|
@raffael0 not sure why the github action does not find any tests to run? |
|
Sorry yeah I fucked that up. I pushed a fix to main |
|
|
||
| #ifndef NO_INFLUX | ||
| logger.Init(config["/INFLUXDB/database_ip"], | ||
| config["/INFLUXDB/database_port"], | ||
| config["/INFLUXDB/database_name"], | ||
| config["/INFLUXDB/sequences_measurement"], MICROSECONDS, | ||
| config["/INFLUXDB/buffer_size"]); | ||
| #endif | ||
| } |
There was a problem hiding this comment.
As much as I hate singletons, I feel like we are at a point of no return in this repo. Wouldn't it be better to use a single logger instance(by turning it into a singleton)?
This kind of splits up the config handling and I do not know if that's a smart thing to do. Feel free to disagree though
There was a problem hiding this comment.
Yeah, i think that would make sense at this point. I don't quite understand why there are so many singletons in the first place, like it surely would be possible to just create the instances in main() and pass them around if needed.
Anyway, we'll need to ensure it is thread-safe however because I think it can be used my multiple threads at the same time. Do you think I should do it in a new PR or here?
There was a problem hiding this comment.
Maybe move it to another PR, since we are already using this in production
raffael0
left a comment
raffael0
left a comment
There was a problem hiding this comment.
LGTM.
We should merge this as soon as possible since we are already using it
Log start/end/abort of sequences, as well as the sequence timer, to influx.
depends on #7 for convenience