-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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(new sink): Initial aws_kinesis_firehose
sink
#1388
Conversation
Signed-off-by: Luke Steensen <[email protected]>
Signed-off-by: Luke Steensen <[email protected]>
Signed-off-by: Luke Steensen <[email protected]>
Signed-off-by: Luke Steensen <[email protected]>
Signed-off-by: Luke Steensen <[email protected]>
aws_kinesis_firehose
sink
Signed-off-by: binarylogic <[email protected]>
Signed-off-by: binarylogic <[email protected]>
Signed-off-by: binarylogic <[email protected]>
Signed-off-by: binarylogic <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs look good
src/sinks/aws_kinesis_firehose.rs
Outdated
|
||
#[derive(Clone)] | ||
pub struct KinesisFirehoseService { | ||
client: Arc<KinesisFirehoseClient>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think we need to arc this https://docs.rs/rusoto_firehose/0.42.0/rusoto_firehose/struct.KinesisFirehoseClient.html#impl-Clone
}; | ||
let common = ElasticSearchCommon::parse_config(&config).expect("Config error"); | ||
|
||
let client = reqwest::Client::builder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda wanna get rid of reqwest and just use the elastic libraries serde feature. But we can do that in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🔥
Signed-off-by: Luke Steensen <[email protected]>
This looks great! I wrote a suggestion about further improving it in #1407. |
Closes #559
Closes #1062
This is another stab at an
aws_kinesis_firehose
sink. It's mostly a copied and modified version of theaws_kinesis_streams
sinks, with some additional copying from theelasticsearch
sink test setup (since we configure it to write to ES for the test).There's probably a good bit of deduplication we could do between the two sinks (and ES tests), but this PR defers all of that in favor of getting something working.
I initially tried to test against an S3 delivery stream, which we should do at some point, but was foiled by some combination of localstack's S3 implementation and rusoto (localstack/localstack#1647).
One thing that's unclear is whether or not we should add an option to encode newlines for delivery streams aimed at S3. I have not looked at how the objects look without it.
External refs: fluent/fluent-bit#2485