Skip to content

correlation: make grouping-by and group-lines clone ids unique among instances sharing a common ancestor #4478

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Sep 21, 2023
Merged

correlation: make grouping-by and group-lines clone ids unique among instances sharing a common ancestor #4478

merged 2 commits into from
Sep 21, 2023

Conversation

alexb271
Copy link
Contributor

Resolves #4443

alltilla
alltilla reviewed May 15, 2023
View reviewed changes
Copy link
Collaborator

@alltilla alltilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also fix group-lines(). Thanks!

@alltilla
Copy link
Collaborator

I'm starting to feel like an incremental clone_id will never provide us the uniqueness we need. Maybe we should think about replacing it with something unique to each clone if that is possible.

@alexb271
Copy link
Contributor Author

I've changed the implementation to a clone_id that is incremented only among instances with a common ancestor. Perhaps that could work?

@alexb271 alexb271 force-pushed the groupby_id branch 2 times, most recently from 4bcffe6 to 8702c4e Compare May 15, 2023 13:48
alltilla
alltilla reviewed May 16, 2023
View reviewed changes
Copy link
Collaborator

@alltilla alltilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this solution :)

However while reviewing your previous one I realized that maybe incremental clone-ids will never solve our issue. I think the user can modify the config so that the new clone happens in the middle of the cloning chain. E.g.:

Let's say the config produces the following clone-id pairing:
A: 1, B: 2, C: 3

The user adds another reference of this parser (D), but it is placed in such a place in the config, where it get's cloned after B. We get the following ids:
A: 1, B: 2, D: 3, C: 4.

Or the user deletes the parser from B and C, and inserts it to D. We get the following ids:
A: 1, D: 2.

This means the clone-id is still dependent on other uses of the parser in the config.

If it is possible we should find a unique identifier of where the cloning happens, so we don't run into this problem. Not sure what it should be or if it is even possible. We should investigate what information is available from the context, where the cloning happens.

If we cannot find such info, I think your implementation is better than the one on master, so we could merge it, but it is still not complete, unfortunately.

@alexb271
Copy link
Contributor Author

I like this solution :)

However while reviewing your previous one I realized that maybe incremental clone-ids will never solve our issue. I think the user can modify the config so that the new clone happens in the middle of the cloning chain. E.g.:

Let's say the config produces the following clone-id pairing: A: 1, B: 2, C: 3

The user adds another reference of this parser (D), but it is placed in such a place in the config, where it get's cloned after B. We get the following ids: A: 1, B: 2, D: 3, C: 4.

Or the user deletes the parser from B and C, and inserts it to D. We get the following ids: A: 1, D: 2.

This means the clone-id is still dependent on other uses of the parser in the config.

If it is possible we should find a unique identifier of where the cloning happens, so we don't run into this problem. Not sure what it should be or if it is even possible. We should investigate what information is available from the context, where the cloning happens.

If we cannot find such info, I think your implementation is better than the one on master, so we could merge it, but it is still not complete, unfortunately.

Thanks!

I think a fully non-incremental solution could be difficult to implement. We cannot rely on the filter's order of occurance or line number in the config, therefore its ID should then be based on a semantic understanding of the config itself.

One shortcut would be to store (or hash) the configuration above the group-by() call, but then irrelevant changes in other sections would also invalidate the persist state.

A full solution would be to understand and store (or hash) the group-by() call's relevant context in the config file, such as the log path it is declared in, or any other possible context.

In my opinion that would be a larger project that might best be handled in a separate enhancement issue.

@kira-syslogng
Copy link
Contributor

Build FAILURE

@alexb271 alexb271 force-pushed the groupby_id branch 6 times, most recently from c3a2edc to 062b2a5 Compare May 16, 2023 15:40
@alexb271 alexb271 changed the title correlation: make grouping-by clone ids unique correlation: make grouping-by clone ids unique among instances sharing a common ancestor May 16, 2023
@alexb271 alexb271 changed the title correlation: make grouping-by clone ids unique among instances sharing a common ancestor correlation: make grouping-by and group-lines clone ids unique among instances sharing a common ancestor May 16, 2023
@MrAnno MrAnno added this to the syslog-ng 4.4 milestone Jul 21, 2023
@MrAnno MrAnno requested a review from alltilla July 24, 2023 16:03
alltilla
alltilla reviewed Jul 31, 2023
View reviewed changes
@alexb271 alexb271 force-pushed the groupby_id branch 4 times, most recently from d0589d2 to f993e58 Compare September 4, 2023 14:31
Alex Becker and others added 2 commits September 4, 2023 16:33
@alexb271
correlation: make grouping-by clone ids unique among instances sharin…
c72b87f 
…g a common ancestor

See here: syslog-ng#4443

Signed-off-by: Alex Becker <alex.becker@quest.com>
@alexb271
correlation: refactor IdCounter lifecycle management according to rev…
97f1115 
…iew feedback

Signed-off-by: Alex Becker <beckeralex@protonmail.com>
@alltilla
Copy link
Collaborator

Thanks! :)

@alltilla alltilla merged commit 94b389d into syslog-ng:master Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
2 more reviewers

@MrAnno MrAnno MrAnno left review comments

@alltilla alltilla alltilla approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
syslog-ng 4.4
Development

Successfully merging this pull request may close these issues.

grouping-by(), group-lines(): persist name is dependent on the number of times it gets cloned
4 participants
@alexb271 @alltilla @kira-syslogng @MrAnno