feat(new transform): New `merge` transform #1504

MOZGIII · 2020-01-09T22:28:04Z

Closes #1488.

MOZGIII · 2020-01-11T13:26:36Z

The feature is incomplete, however, the approach is settled, and overall it's ready for review. What remains to be added is:

built-in merge transform for journald;
built-in merge transform for kubernetes (to match docker);
a transform for regexp-based partial event marker implementations (a simple one, for advanced cases user can already use lua transform);
support for multiple partial event queues in merge transform to enable use in service deployment scenarios.

binarylogic · 2020-01-11T16:01:24Z

Nice work! Do you plan to do those remaining items in this PR or in separate PRs?

MOZGIII · 2020-01-11T16:08:44Z

I'd move some of the pending things from the checklist to the separate PRs:

journald - I'm currently looking into it, and it might be a bit more complicated than I anticipated originally;
a custom regexp-based partial events marker- transform - it's unclear wherher this is even needed, since it might be just a couple line of lua;
kubernetes - it might be non-trivial too, I'd submit a separate PR for it; I wanted to take care of it as part of this PR, but now I think it can be clearly split since the implementation is very different there.

I'd like to take care of the last point - multiple partial event queues - as part of this one.

MOZGIII · 2020-01-11T16:09:30Z

Btw, I think it's time to team up for test harness setup.

Signed-off-by: MOZGIII <[email protected]>

…d, ignoring the value Signed-off-by: MOZGIII <[email protected]>

Signed-off-by: MOZGIII <[email protected]>

…te of the value Signed-off-by: MOZGIII <[email protected]>

binarylogic · 2020-01-11T16:14:09Z

Sounds good. Also, regarding the journald work, we are making some changes there via #1473. I want to make sure that does not conflict with your work.

Let's schedule time next week to dive into the test harness. I'll ping you.

…ier understanding Signed-off-by: MOZGIII <[email protected]>

Signed-off-by: MOZGIII <[email protected]>

We'll most likely be reusing that logic pretty soon. Signed-off-by: MOZGIII <[email protected]>

Signed-off-by: MOZGIII <[email protected]>

binarylogic · 2020-02-04T18:21:10Z

@MOZGIII mind resolving the conflicts? Then hopefully we can get this merged 🚀

MOZGIII · 2020-02-04T18:24:12Z

@binarylogic sure, will do shortly. I'd like to have feedback from @lukesteensen on #1504 (comment) before the merge.
Also, another pass on the documentation is required, I can do that too.

lukesteensen · 2020-02-04T19:47:55Z

Yep, I think if we can get this green on CI then we should be in a good place to merge.

binarylogic · 2020-02-04T19:50:27Z

Nice! Let's make the final changes in #1504 (comment) and then we can merge @MOZGIII .

It's not as optimized at the previous variant, however it's way easier to understand and maintain. Signed-off-by: MOZGIII <[email protected]>

MOZGIII · 2020-02-04T21:01:50Z

There are some merge conflicts that are in the .meta/transforms/concat.toml. The changes to that have initially arrived to this branch from 1421277#diff-08a736386e72041abbb1de9fb9000216
I'm not sure how relevant are they, and what would be a reasonable resolution in the context iof this PR at this point.
Same applies to website/metadata.js.

Signed-off-by: MOZGIII <[email protected]>

MOZGIII · 2020-02-04T21:15:45Z

Ideally, we should alter that commit (1421277) to only include the changes relevant to this PR.

Signed-off-by: MOZGIII <[email protected]>

MOZGIII · 2020-02-04T21:48:20Z

Ended up with a merge resolution that negates the irrelevant changes, not sure how correct that is.

Signed-off-by: MOZGIII <[email protected]>

binarylogic · 2020-02-04T22:05:10Z

That's fine. I'll review to make sure nothing is off.

MOZGIII · 2020-02-04T22:10:16Z

Thanks @binarylogic! I've tried to solve the check-blog, and pretty much succeeded, but it seems there's an ongoing work there - so I'll just leave it to you. Sorry if I messed something up already.

binarylogic · 2020-02-04T22:27:13Z

I'll do a final pass on this, get the checks passing, and then merge.

MOZGIII · 2020-02-04T22:28:53Z

Just one sec, I'll update the docs. I'm only supposed to update the text at .meta dir manually, correct?

binarylogic · 2020-02-04T22:29:36Z

Yep, modify everything in the /.meta folder then run make generate.

- Pull the latest field descriptions from the code for merge transform - Update the field names at merge transform and docker source - Update schema to allow `merge` at the `function_category` - Remove `guides` and `resources` from merge transform - not sure what those are about - Fix some minor nitpicks Signed-off-by: MOZGIII <[email protected]>

Signed-off-by: MOZGIII <[email protected]>

MOZGIII · 2020-02-04T22:47:11Z

Done! So, one concern I had is with 687e281#diff-cc6d35117dc2abae06d7b7f10c958363L6-L9 - the guides and resources field, I don't know where those come from, and the schema doesn't allow them. That's why I though there's an ongoing process of some kind there.

Signed-off-by: binarylogic <[email protected]>

anton-ryzhov · 2020-02-07T14:50:18Z

website/docs/reference/transforms/merge.md

+
+Where possible, Vector will handle event merging at the source level. For
+example, the [`file`][docs.sources.file] contains a `message_start_indicator`
+option and the [`docker`][docs.sources.file] contains an `auto_partial_merge`


@binarylogic here is a wrong link, should be [`docker`][docs.sources.docker]

alexgavrisco · 2020-03-24T16:55:51Z

Please forgive my ignorance in case it's been already answered, I couldn't find the answer in this PR or other issues.
@MOZGIII @binarylogic I've encountered the same issue with kubernetes source. And it doesn't seem that I can achieve it using the merge filter since I'm not in control of splitting messages (kubernetes is). Do you have any pointers for this issue?

MOZGIII · 2020-03-24T16:58:19Z

@Alexx-G thanks for reaching out! You're looking for this: #1718
I'll be working on it this week!

alexgavrisco · 2020-03-24T18:21:46Z

Yep, this is exactly what I'm looking for. Can't believe I missed.
Thanks a lot! Subscribed for updates.

binarylogic requested a review from lukesteensen January 9, 2020 23:41

binarylogic assigned lukesteensen Jan 9, 2020

MOZGIII force-pushed the merge-transform branch 3 times, most recently from a9dc7ff to 9147835 Compare January 10, 2020 12:48

MOZGIII mentioned this pull request Jan 10, 2020

Docker: use shiplift's since instead of manually filtering the logs #1506

Closed

MOZGIII force-pushed the merge-transform branch from 997ac28 to 1e9f355 Compare January 10, 2020 16:20

MOZGIII marked this pull request as ready for review January 10, 2020 20:05

MOZGIII requested a review from LucioFranco as a code owner January 10, 2020 20:05

binarylogic changed the title ~~feat(new transform): Merge transform~~ feat(new transform): New merge transform Jan 11, 2020

binarylogic removed the request for review from LucioFranco January 11, 2020 16:00

MOZGIII added 7 commits January 11, 2020 19:13

Add merge transform

69b4ad6

Signed-off-by: MOZGIII <[email protected]>

Mark field at partial by solely by the presence of the indicator fiel…

0796291

…d, ignoring the value Signed-off-by: MOZGIII <[email protected]>

Flag partial messages at the docker source

59ab3dd

Signed-off-by: MOZGIII <[email protected]>

Improved the documentation on the config params

f1c7fff

Signed-off-by: MOZGIII <[email protected]>

Remove the partial indicator field when we stash partial events

ca3993f

Signed-off-by: MOZGIII <[email protected]>

Rename merge to merge_event

9988784

Signed-off-by: MOZGIII <[email protected]>

Added more test cases and fixed an issue with losing explicitness sta…

5d562a3

…te of the value Signed-off-by: MOZGIII <[email protected]>

MOZGIII added 7 commits January 11, 2020 19:16

Removed duplicating comments at DockerSourceCore and reworded for eas…

1065241

…ier understanding Signed-off-by: MOZGIII <[email protected]>

Add support for default values for serde

b5c2bd0

Signed-off-by: MOZGIII <[email protected]>

Made partial event marker configurable per docker source instance

f18c643

Signed-off-by: MOZGIII <[email protected]>

Rename partial_event_indicator_field to partial_event_marker

9c45e5e

Signed-off-by: MOZGIII <[email protected]>

Optimized partial marker check by using a removal value

96b1db7

Signed-off-by: MOZGIII <[email protected]>

Moved the merging logic to a separate mod under the event mod

0045aff

We'll most likely be reusing that logic pretty soon. Signed-off-by: MOZGIII <[email protected]>

Fixed typos at explicit removal TODO

df800a2

Signed-off-by: MOZGIII <[email protected]>

Switched back to the simpler logic of partial event merging

2359d2b

It's not as optimized at the previous variant, however it's way easier to understand and maintain. Signed-off-by: MOZGIII <[email protected]>

Merge branch 'master' into merge-transform

8c42082

Signed-off-by: MOZGIII <[email protected]>

fixup! Switched back to the simpler logic of partial event merging

c3eb2b4

Signed-off-by: MOZGIII <[email protected]>

Fix fmt at the website

24d300c

Signed-off-by: MOZGIII <[email protected]>

lukesteensen approved these changes Feb 4, 2020

View reviewed changes

MOZGIII added 2 commits February 5, 2020 01:44

Run make generate

b2e53b9

Signed-off-by: MOZGIII <[email protected]>

Update documentation

b346a83

Signed-off-by: binarylogic <[email protected]>

binarylogic approved these changes Feb 5, 2020

View reviewed changes

binarylogic merged commit c285303 into master Feb 5, 2020

binarylogic deleted the merge-transform branch February 5, 2020 05:14

This was referenced Feb 5, 2020

Merge back docker logs #1436

Closed

message_stop_indicator for file source #1431

Closed

anton-ryzhov reviewed Feb 7, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(new transform): New `merge` transform #1504

feat(new transform): New `merge` transform #1504

MOZGIII commented Jan 9, 2020

MOZGIII commented Jan 11, 2020 •

edited

Loading

binarylogic commented Jan 11, 2020

MOZGIII commented Jan 11, 2020

MOZGIII commented Jan 11, 2020

binarylogic commented Jan 11, 2020

binarylogic commented Feb 4, 2020

MOZGIII commented Feb 4, 2020

lukesteensen commented Feb 4, 2020

binarylogic commented Feb 4, 2020

MOZGIII commented Feb 4, 2020

MOZGIII commented Feb 4, 2020

MOZGIII commented Feb 4, 2020

binarylogic commented Feb 4, 2020

MOZGIII commented Feb 4, 2020

binarylogic commented Feb 4, 2020

MOZGIII commented Feb 4, 2020

binarylogic commented Feb 4, 2020

MOZGIII commented Feb 4, 2020

anton-ryzhov Feb 7, 2020

alexgavrisco commented Mar 24, 2020

MOZGIII commented Mar 24, 2020

alexgavrisco commented Mar 24, 2020

feat(new transform): New merge transform #1504

feat(new transform): New merge transform #1504

Conversation

MOZGIII commented Jan 9, 2020

MOZGIII commented Jan 11, 2020 • edited Loading

binarylogic commented Jan 11, 2020

MOZGIII commented Jan 11, 2020

MOZGIII commented Jan 11, 2020

binarylogic commented Jan 11, 2020

binarylogic commented Feb 4, 2020

MOZGIII commented Feb 4, 2020

lukesteensen commented Feb 4, 2020

binarylogic commented Feb 4, 2020

MOZGIII commented Feb 4, 2020

MOZGIII commented Feb 4, 2020

MOZGIII commented Feb 4, 2020

binarylogic commented Feb 4, 2020

MOZGIII commented Feb 4, 2020

binarylogic commented Feb 4, 2020

MOZGIII commented Feb 4, 2020

binarylogic commented Feb 4, 2020

MOZGIII commented Feb 4, 2020

anton-ryzhov Feb 7, 2020

Choose a reason for hiding this comment

alexgavrisco commented Mar 24, 2020

MOZGIII commented Mar 24, 2020

alexgavrisco commented Mar 24, 2020

feat(new transform): New `merge` transform #1504

feat(new transform): New `merge` transform #1504

MOZGIII commented Jan 11, 2020 •

edited

Loading