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

Don't try to aggregate content with a NOQUEUE queue id #190

Merged
merged 1 commit into from Jan 29, 2024

Conversation

whyscream
Copy link
Owner

After the comment by @ulab in #150.

@whyscream whyscream merged commit 11a75cf into main Jan 29, 2024
1 check passed
@whyscream whyscream deleted the 150-exclude-noqueue-from-aggregation branch January 29, 2024 21:30
@ulab
Copy link
Contributor

ulab commented Jan 29, 2024

I am not sure this was the right solution. The NOQUEUE was added to resolve issue #150 and avoid losing those lines in the logs in the first place 😄.

I do think a better solution would be to extend the conditionals with an and [postfix_queueid] != "NOQUEUE" or similar, but I use Kibana Pipelines instead of Logstash, so I am not if that would work nor could I test it.

@whyscream
Copy link
Owner Author

whyscream commented Feb 2, 2024

I thought this over. What you want is a noop, that just ignores the event. I do think that we blanket ignore NOQUEUE events here, as there is no way to aggregate them anyway.
I found elastic/logstash#3493, which explains that a mutate (without any actual mutations I guess) would do exactly the same. Which sounds unclear but useful.

@ulab
Copy link
Contributor

ulab commented Feb 2, 2024

I wonder if you can just -uh- drop the drop() and have an empty if clause? I don't have logstash to test if that would work.

Coming from Ingest pipelines (that can't do aggregations like this 😞 ), I don't think I can do if … else either, so I would've had to combine the conditions like this

if [postfix_queueid] and [postfix_queueid] != "NOQUEUE" and [program] == "postfix/qmgr" and [postfix_from] {
   aggregate...
}
if [postfix_queueid] and [postfix_queueid] != "NOQUEUE" and [program] == "postfix/smtpd" {
   aggregate...

It doesn't look as clean, but should work without a workaround or plugin.

@whyscream
Copy link
Owner Author

whyscream commented Feb 4, 2024

I setup a local ELK stack to test some stuff using https://github.com/deviantony/docker-elk.

  1. Using an empty if clause is perfectly acceptable, no empty mutate necessary :)
  2. The events with a NOQUEUE queueid, and the events without a queueid are indeed removed. This seems harmful.
  3. The current aggregation example file didn't not work out of the box for me: the program names are hardcoded and don't support postfix instance names. The 50-filter-postfix.conf file supports these.
  4. When investigating how aggregation should actually work, I feel that the example misses some stuff.
    • The elastic docs (https://www.elastic.co/guide/en/logstash/current/plugins-filters-aggregate.html), plugin version v2.10.0) state that a map is created that contains the aggregated data. When a final event occurs, you can do something with this map (such as attaching it to the final event). However there is no final event in the example, nor any of the configuration for using a timeout trigger.
    • The things that the example does show, is just basic aggregation stuff that you can find in the elastic docs. There is nothing in there that does something special for postfix logging. If you already know and use aggregation, this example won't help or learn you anything. If you're new to it, the elastic docs will tell the exact same stuff, but then complete and with proper explanation.

I'm now leaning to removing the whole aggregation stuff, unless we try to replicate the collate script that is part of the postfix code. However, I don't see any reason to replicate anything.

@ulab
Copy link
Contributor

ulab commented Feb 4, 2024

I agree. With these issues I wonder if anyone really used it anyway since nobody seems to have noticed/reported before?

@whyscream
Copy link
Owner Author

Done in #194

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.

None yet

2 participants