Centralized internal logging #364
Conversation
OverOrion
force-pushed
the
debug-add-msg-rcptid
branch
3 times, most recently
from
7912ede to
af419c1
Compare
lib/filter/filter-call.c
Outdated
| evt_tag_str("called-rule", self->rule), | ||
| evt_tag_msg_reference(msgs[num_msg - 1])); | ||
| evt_tag_str("called-rule", self->rule)); | ||
| msg_set_context(NULL); |
There was a problem hiding this comment.
This will remove the reference to the current message, is that really a good idea? the next filter would stop printing the rcptid this way.
I don't think either msg_set_context( ) calls are needed here.
There was a problem hiding this comment.
I don't think we need to set the context this many times. Every case, the current rcptid will be the same as there's only one message being handled at any given time.
Since we are not printing the pointer anymore, I don't think any of the msg_set_context() calls are needed in this patch. Let's discuss.
There was a problem hiding this comment.
Attila and I discussed this IRL, here is the summarized version:
msg_set_context()calls should be placed at places where newLogMessageinstances are encountered. It's constructor seemed a viable option at first, however that would introduce a layering violation so we opted for moving it elsewhere.- During our internal queuing where
pop()calls happen is the moment in time where logs are encountered before being sent to destinations. This combined with setting context at insertions (reading from source) seemed like the best place for them. LogSchedulerhas it's very own (and unique) way of fetching logs, that's why it required a separate patch (easy to miss it) .- We could not move it inside
log_source_postas there are already logs printed before the call actually happens.
OverOrion
force-pushed
the
debug-add-msg-rcptid
branch
3 times, most recently
from
2634546 to
b024c26
Compare
The definition was removed in a276841, but the declaration was not. Signed-off-by: Szilard Parrag <[email protected]>
This was required as the aim is to centralize logging. Signed-off-by: Szilard Parrag <[email protected]>
This is needed as there has to be central place where this is logged. Signed-off-by: Szilard Parrag <[email protected]>
This is the followup for the previous patch, making evt_tag_msg_reference obsolete. This patch also removes printing memory addresses. Signed-off-by: Szilard Parrag <[email protected]>
It's done as their values will be overwritten anyway. Signed-off-by: Szilard Parrag <[email protected]>
It's done to ensure the following logging calls have the correct rcptid. Signed-off-by: Szilard Parrag <[email protected]>
This patch adds msg_set_context calls to places where LogMessages are popped from the queue. This happens after a message has been "read"/"consumed" (from the source side) but before it is sent (destination pops from the queue). Signed-off-by: Szilard Parrag <[email protected]>
This is required here as the LogMessage popping is done rather oddly. Signed-off-by: Szilard Parrag <[email protected]>
This is done as the previous commits set the context when the messages are popped from the queue. Signed-off-by: Szilard Parrag <[email protected]>
Signed-off-by: Szilard Parrag <[email protected]>
Signed-off-by: Szilard Parrag <[email protected]>
This is done as the subclasses already set it themselves, making this call redundant. Signed-off-by: Szilard Parrag <[email protected]>
Signed-off-by: Szilard Parrag <[email protected]>
OverOrion
force-pushed
the
debug-add-msg-rcptid
branch
from
b024c26 to
02093f9
Compare
| @@ -123,6 +123,7 @@ syslogng::grpc::otel::TraceServiceCall::Proceed(bool ok) | |||
| } | |||
|
|
|||
| LogMessage *msg = log_msg_new_empty(); | |||
| msg_set_context(msg); | |||
There was a problem hiding this comment.
We want to track the message we are currently working on globally (per-thread). This idea will always imply a certain amount of layering violation, but placing this call into LogMessage constructors wouldn't necessarily help as I'm not sure we want to track each message that pops into existence.
For example, what do we want to do with synthetic messages produced by the grouping-by() and PatternDB parsers? And internal messages of course, which we don't want to track.
There was a problem hiding this comment.
Once this question is answered, I think we can introduce a function that can help reduce these set_context calls at least on the source and processing side.
| @@ -489,6 +489,7 @@ log_queue_fifo_pop_head(LogQueue *s, LogPathOptions *path_options) | |||
| if (!node->flow_control_requested) | |||
| self->backlog_queue.non_flow_controlled_len++; | |||
|
|
|||
| msg_set_context(msg); | |||
There was a problem hiding this comment.
Why don't you place this in the log queue abstract pop method? That way, the memory queues and disk buffers won't have to be careful about this.
| @@ -151,6 +151,7 @@ _fetch(LogThreadedFetcherDriver *s) | |||
|
|
|||
| gint64 remaining_messages = log_queue_get_length(self->queue); | |||
| LogMessage *msg = log_queue_pop_head(self->queue, &local_options); | |||
| msg_set_context(msg); | |||
There was a problem hiding this comment.
This can be removed once the log queue abstract class does its job.
|
We've discussed this with @bazsi IRL and came to the conclusion that this might not worth the effort (considering that this does not cause any problems), so closing this as is, but can be revisited if needed. |
No description provided.