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

feat(exclusive_route transform): implements new transform #21707

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

pront
Copy link
Contributor

@pront pront commented Nov 5, 2024

Summary

Introduces the new exclusive route transform. This is transform is very similar to the route transform but it acts like switch statement for routes (vs matching more than one route).

We did consider adding extra options to the existing route transform, but the route transform has an unordered list of routes and so we'd need to further complicate the UX of that transform by either adding a weight parameter to each route to order them (opening a question of what to do if the weights are the same for two routes) or by migrating the route configuration from a map to a list (while ideally maintaining backwards compatibility).

Change Type

  • Bug fix
  • New feature
  • Non-functional (chore, refactoring, docs)
  • Performance

Is this a breaking change?

  • Yes
  • No

How did you test this PR?

Does this PR include user facing changes?

  • Yes. Please add a changelog fragment based on our guidelines.
  • No. A maintainer will apply the "no-changelog" label to this PR.

Checklist

References

@github-actions github-actions bot added domain: transforms Anything related to Vector's transform components domain: external docs Anything related to Vector's external, public documentation labels Nov 5, 2024
@pront
Copy link
Contributor Author

pront commented Nov 5, 2024

TODO: add CUE doc. Opening it up for comments.

@pront pront marked this pull request as ready for review November 5, 2024 21:15
@pront pront requested review from a team as code owners November 5, 2024 21:15
@datadog-vectordotdev
Copy link

datadog-vectordotdev bot commented Nov 5, 2024

Datadog Report

Branch report: pront/exclusive-route-transform
Commit report: b0df5b1
Test service: vector

✅ 0 Failed, 7 Passed, 0 Skipped, 25.46s Total Time

Copy link
Member

@jszwedko jszwedko left a comment

Choose a reason for hiding this comment

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

Nice! I left a few comments below, but otherwise this looks good to me. I considered some other possible names for this transform (branch, switch, split, etc.) but I think the exclusive_route name makes its relationship to route clear.

@@ -0,0 +1,29 @@
package metadata
Copy link
Member

Choose a reason for hiding this comment

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

Note that, in addition to the cue doc you mentioned is still TODO, you'll need to create a markdown file for the transform too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

create a markdown file for the transform too.

Updated the PR with a lot of doc related changes. Not sure about this comment. Let me know if I missed something 🙏

Copy link
Member

Choose a reason for hiding this comment

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

I think you are still missing the markdown file so the new transform docs won't be rendered on the website. You'll need to create website/content/en/docs/reference/configuration/transforms/exclusive_route.md. Hugo uses that to generate the transform page (theoretically these pages could be generated from the cue docs, I think, but they currently aren't).

@@ -180,6 +180,7 @@ jobs:
reduce transform
remap transform
route transform
exclusive_route transform
Copy link
Member

Choose a reason for hiding this comment

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

👍 appreciate you adding this at the same time.

@@ -0,0 +1,3 @@
Introduce a new exclusive_route transform, which functions as a switch statement to route events based on user-defined conditions.
Copy link
Member

Choose a reason for hiding this comment

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

I think you mentioned you'd be writing a blog post on this one. I think a release highlight might be appropriate too (or instead of the blog): https://vector.dev/highlights/.

src/transforms/exclusive_route/config.rs Outdated Show resolved Hide resolved
src/transforms/exclusive_route/config.rs Outdated Show resolved Hide resolved
Copy link
Member

@jszwedko jszwedko left a comment

Choose a reason for hiding this comment

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

Thanks for the updates!

It looks like website/cue/reference/components/transforms/exclusive_route.cue is empty 🤔


if !duplicates.is_empty() {
errors.push(format!(
"Found routes with duplicate names: {:?}",
Copy link
Member

Choose a reason for hiding this comment

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

👍

@pront
Copy link
Contributor Author

pront commented Nov 7, 2024

Thanks for the updates!

It looks like website/cue/reference/components/transforms/exclusive_route.cue is empty 🤔

ACK, I will add it soon.

@jorgehermo9
Copy link
Contributor

jorgehermo9 commented Nov 9, 2024

have you thought about adding a exclusive: true | false flag to the existing route transform? is it worth to add a completely new one for this usecase?

@jszwedko
Copy link
Member

jszwedko commented Nov 11, 2024

have you thought about adding a exclusive: true | false flag to the existing route transform? is it worth to add a completely new one for this usecase?

We did consider that, but the existing route transform has an unordered list of routes and so we'd need to further complicate the UX of that transform by either adding a weight parameter to each route to order them (opening a question of what to do if the weights are the same for two routes) or by migrating the route configuration from a map to a list (while ideally maintaining backwards compatibility).

@pront
Copy link
Contributor Author

pront commented Nov 11, 2024

have you thought about adding a exclusive: true | false flag to the existing route transform? is it worth to add a completely new one for this usecase?

We did consider that, but the existing route transform has an unordered list of routes and so we'd need to further complicate the UX of that transform by either adding a weight parameter to each route to order them (opening a question of what to do if the weights are the same for two routes) or by migrating the route configuration from a map to a list (while ideally maintaining backwards compatibility).

Well said, added this to the PR summary 👍

@pront pront requested a review from jszwedko November 12, 2024 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain: external docs Anything related to Vector's external, public documentation domain: transforms Anything related to Vector's transform components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to make routes exclusive
4 participants