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

rewrite gcp_pubsub output #1836

Merged
merged 1 commit into from
May 10, 2023
Merged

Conversation

disintegrator
Copy link
Collaborator

@disintegrator disintegrator commented Apr 6, 2023

This change rewrites the gcp_pubsub output to use the new config API and to lean on the pubsub client's internal batching mechanism. This is referred to as the "internal buffer" in docs to distinguish it from Benthos' batching mechanism. The rewrite also benefits from added tests and a pubsub mock.

Stress test results on an M1 Pro (8-core, 16GB mem):

prometheus metric graph of rate(output_sent[30s]) for today's gcp_pubsub output, peaking at around 1.7K messages per second, and the new one in this pull request, peaking at around 10K mesages per second

(gc_gcp_pubsub is the working name of the output in this PR that doesn't clash with the existig output)

@disintegrator disintegrator requested a review from Jeffail as a code owner April 6, 2023 23:05
@disintegrator disintegrator marked this pull request as draft April 6, 2023 23:06
"github.com/benthosdev/benthos/v4/internal/message"
"github.com/benthosdev/benthos/v4/internal/metadata"
"github.com/benthosdev/benthos/v4/public/service"
"github.com/sourcegraph/conc/pool"
Copy link
Collaborator Author

@disintegrator disintegrator Apr 6, 2023

Choose a reason for hiding this comment

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

This library has a useful result pool, kind of like of Promise.allSettled. If it's a no go then I'd be happy to put something together with errgroup or similar.

https://pkg.go.dev/github.com/sourcegraph/conc/pool#ResultPool

Comment on lines +73 to +75
service.NewMetadataFilterField("metadata").
Optional().
Description("Specify criteria for which metadata values are sent as attributes."),
Copy link
Collaborator Author

@disintegrator disintegrator Apr 6, 2023

Choose a reason for hiding this comment

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

This is a breaking change because the old api added metadata.exclude_prefixes whereas the new api added metadata.include_prefixes and metadata.include_patterns. Should I manually implement the old config somehow?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I might expand the public field definition so that you can toggle exclusive versus inclusive defaults. Ideally all components will eventually converge but we need to do it in stages (with backwards compatibility).

@disintegrator disintegrator force-pushed the gcp-pubsub-next branch 4 times, most recently from 84d60aa to 854e5f6 Compare April 7, 2023 00:00
@disintegrator disintegrator force-pushed the gcp-pubsub-next branch 9 times, most recently from 34604a4 to 9e35295 Compare April 14, 2023 13:17
@disintegrator disintegrator marked this pull request as ready for review April 14, 2023 13:39
This change rewrites the gcp_pubsub output to use the new config API and
to lean on the pubsub client's internal batching mechanism. This is
referred to as the "internal buffer" in docs to distinguish it from
Benthos' batching mechanism. The rewrite also benefits from added tests
and a pubsub mock.

Signed-off-by: Georges Haidar <[email protected]>
Copy link
Collaborator

@Jeffail Jeffail left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @disintegrator, going to switch to the new NewMetadataExcludeFilter to fill the gap in backwards compat.

@Jeffail Jeffail merged commit aafc739 into redpanda-data:main May 10, 2023
@disintegrator disintegrator deleted the gcp-pubsub-next branch May 10, 2023 09:08
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.

2 participants