chore(sinks): Refactor internal sinks and simplify #2111

LucioFranco · 2020-03-20T21:10:39Z

This is a decently large refactor PR of our internal sink utilities. Reference #1666 for motivation for this change. At a high level this change introduces three new types StreamSink, BatchSink and PartitionBatchSink. These types are similar to our current types except for instead of wrapping sinks (StreamSink still takes an inner sink) and causing a large type mess, we upfront expose all trait bounds and do not allow users to nest things. This achieves the same goals as we had previously but with much better compiler errors and a much simpler implementation.

Both batching sinks now take a Service instead of some inner sink, which now allows us to surface trait bounds at the root level. Before, we used extension traits that would cause crazy error messages if you missed one slight bound.

Another benefit to this change is that we now allow all requests to be dispatched within their own task which should allow them to be driven independently from the sink. This should solve #299.

Other things added are, improved testing around acking, lingers and batching. Improved tracing and docs explaining what is going on under the hood. We have also unified all sinks to live under one file now instead of within 3 different places. This should clean up our sink util modules.

Closes #299, #1666, and #2049.

Signed-off-by: Lucio Franco <[email protected]>

binarylogic · 2020-03-20T21:28:18Z

Thanks. Would you mind elaborating on the bugs that you discovered? I'd like to open issues detailing them for posterity. This helps when users search for issues in our repo. This PR should close those issues.

LucioFranco · 2020-03-20T22:55:06Z

@binarylogic Besides the IO issue which should be fixed in this PR, there were just a couple deep implementation issues with our batching. I don't think we have seen any of them since no one has reported hanging of tasks. So I don't think its worth it just yet to write issues for them since they are far from user facing bugs but more so potential things that could happen.

Once, I get a bit further with this PR I'll write up a proper description for the PR and some better docs around batching.

…tition

Signed-off-by: Lucio Franco <[email protected]>

…tition

LucioFranco · 2020-03-24T20:37:01Z

Update, there are a few things left to do:

subtle bug with loki and logdna sinks
Vendor our own version of tower-timeout that uses Into implementation
Implement custom downcast_ref for retrylogic
nix old code
Add docs for sinks

Signed-off-by: Lucio Franco <[email protected]>

…tition Signed-off-by: Lucio Franco <[email protected]>

Signed-off-by: Lucio Franco <[email protected]>

LucioFranco · 2020-03-26T15:45:47Z

@lukesteensen @Hoverbear this should be ready for review! I've updated the main PR description.

bruceg

I see nothing majorly objectionable here, though it's much too large a change for me to grasp all the implications of it.

bruceg · 2020-03-26T20:29:02Z

src/sinks/elasticsearch.rs

-            BatchedHttpSink::new(common, Buffer::new(gzip), request, batch, tls_settings, &cx);
+            BatchedHttpSink::new(common, Buffer::new(gzip), request, batch, tls_settings, &cx)
+                .sink_map_err(|e| error!("Fatal elasticsearch sink error: {}", e));


I notice a number of the new sinks add this error handler. Would it make sense to have that within the generic sink impls?

Possibly, though it doesn't totally hurt here, I think in the future we want to take a different approach. At least this forces this to be handled per sink so its fine for now.

src/sinks/util/sink.rs

Signed-off-by: Lucio Franco <[email protected]>

…tition Signed-off-by: Lucio Franco <[email protected]>

Signed-off-by: Lucio Franco <[email protected]>

Hoverbear · 2020-03-30T21:07:52Z

src/sinks/util/sink.rs

+/// batches have been acked. This means if sequential requests r1, r2,
+/// and r3 are dispatched and r2 and r3 complete, all events contained
+/// in all requests will not be acked until r1 has completed.
+pub struct BatchSink<S, B, Request, E = DefaultExecutor> {


I would be totally okay with these being separate files, but it's not a blocker.

I am happy to do that, the big reason I decided to keep it one file is because this is just utility code. @lukesteensen any preference?

It's just a lot of code!

I agree it's a lot of code, but I don't feel too strongly either way.

src/sinks/util/sink.rs

Hoverbear

Rest looks good!

lukesteensen

Seems good! One thing that made this a little difficult to review was figuring out what in StreamSink, BatchSink, PartitionedBatchSink is new vs just moved/renamed. It's a lot to review in-depth, so I want to make sure we focus on what's actually changing.

src/sinks/util/http.rs

lukesteensen · 2020-03-30T20:26:28Z

src/sinks/util/service.rs

+/// We require our own timeout layer because the current
+/// 0.1 version uses From intead of Into bounds for errors
+/// this casues the whole stack to not align and not compile.
+/// In future versions of tower this should be fixed.


Is there an issue we can follow for this? Would be great not to have to maintain this ourselves.

This has been fixed in the newer tower releases that we have not upgraded yet too.

The closest issue I can find is this one tower-rs/tower#387

src/sinks/util/sink.rs

lukesteensen · 2020-03-30T21:49:04Z

src/sinks/util/sink.rs

+        // Apply back pressure if we are buffering more than
+        // 5 batches, this should only happen if the inner sink
+        // is apply back pressure.
+        if self.sending.len() > 5 {


Wouldn't this cause backpressure very quickly if we have data with more than 5 partitions? I'm not quite sure what the goal is here.

This was set before, I was thinking about this but I actually don't think its even possible to go beyond 1 here since the semantics of service. I will take a look tomorrow.

Ok, so I looked into this more the reason I added it originally was to work around multiple batches being ready and the service applying back pressure. Since, we can fill multiple batches at once we want to buffer the complete ones until we can send them so we can continue to fill. Since, in theory the service should only be applying back pressure for a short amount of time this should only live here for a short period.

I also want to avoid refactoring this piece since it seems to be working correctly and we have not noticed issues. I suspect a lot of this code wil go away in the future and replaced by nicer async/await so I want to avoid investing too much. A lot of these types of work arounds are for the sink trait.

Co-Authored-By: Ana Hobden <[email protected]>

LucioFranco · 2020-03-30T22:39:01Z

Thanks for the reviews @Hoverbear and @lukesteensen I'll fix this up with the feedback first thing tomorrow morning.

Signed-off-by: Lucio Franco <[email protected]>

…tition Signed-off-by: Lucio Franco <[email protected]>

LucioFranco added 3 commits March 19, 2020 11:49

wip

012305d

Signed-off-by: Lucio Franco <[email protected]>

wip

56b3e56

Signed-off-by: Lucio Franco <[email protected]>

wip

9309510

Signed-off-by: Lucio Franco <[email protected]>

Hoverbear assigned LucioFranco Mar 23, 2020

Hoverbear added the type: enhancement A value-adding code change that enhances its existing functionality. label Mar 23, 2020

LucioFranco added 3 commits March 24, 2020 11:13

Merge remote-tracking branch 'origin/master' into lucio/http-sink-par…

5300e62

…tition

Refactor all sinks

ec27d6c

Signed-off-by: Lucio Franco <[email protected]>

Merge remote-tracking branch 'origin/master' into lucio/http-sink-par…

fd2a345

…tition

LucioFranco mentioned this pull request Mar 24, 2020

Dynamic connection load balancing for HTTP sinks #2120

Closed

LucioFranco added 7 commits March 25, 2020 12:52

fix false readiness

595b214

Signed-off-by: Lucio Franco <[email protected]>

all tests passing!

0f8f872

Signed-off-by: Lucio Franco <[email protected]>

Merge remote-tracking branch 'origin/master' into lucio/http-sink-par…

2815343

…tition Signed-off-by: Lucio Franco <[email protected]>

Vendor tower-timeout

43d2a1f

Signed-off-by: Lucio Franco <[email protected]>

nix old batching types and refactor buffer tests

4159347

Signed-off-by: Lucio Franco <[email protected]>

add docs

a2c7679

Signed-off-by: Lucio Franco <[email protected]>

add tracing

039a96a

Signed-off-by: Lucio Franco <[email protected]>

LucioFranco marked this pull request as ready for review March 26, 2020 15:45

LucioFranco requested a review from a user March 26, 2020 15:45

LucioFranco requested review from bruceg and lukesteensen as code owners March 26, 2020 15:45

LucioFranco requested a review from Hoverbear March 26, 2020 15:51

bruceg reviewed Mar 26, 2020

View reviewed changes

Hoverbear self-assigned this Mar 30, 2020

LucioFranco added 3 commits March 30, 2020 12:30

fix partition spelling

70ed3c6

Signed-off-by: Lucio Franco <[email protected]>

Merge remote-tracking branch 'origin/master' into lucio/http-sink-par…

6a93f42

…tition Signed-off-by: Lucio Franco <[email protected]>

fix compile

96894f5

Signed-off-by: Lucio Franco <[email protected]>

Hoverbear reviewed Mar 30, 2020

View reviewed changes

src/sinks/util/sink.rs Outdated Show resolved Hide resolved

Hoverbear approved these changes Mar 30, 2020

View reviewed changes

lukesteensen reviewed Mar 30, 2020

View reviewed changes

Update src/sinks/util/sink.rs

bee20cd

Co-Authored-By: Ana Hobden <[email protected]>

address PR comments

7eb3f80

Signed-off-by: Lucio Franco <[email protected]>

LucioFranco requested a review from lukesteensen March 31, 2020 15:19

lukesteensen approved these changes Mar 31, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into lucio/http-sink-par…

0574a51

…tition Signed-off-by: Lucio Franco <[email protected]>

LucioFranco merged commit a2ea56c into master Apr 1, 2020

LucioFranco deleted the lucio/http-sink-partition branch April 1, 2020 17:15

binarylogic mentioned this pull request Apr 1, 2020

enhancement(kubernetes source): Upgrade kube and k8s-openapi crate #2135

Closed

This was referenced Apr 6, 2020

Allow partition batching for HttpSink #2049

Closed

Reduce complexity around batching sinks #1666

Closed

This was referenced May 21, 2020

[aws_s3] vector is unable to start if ec2 instance does not have rw to S3 bucket. #2623

Closed

fix(sinks): ack and don't stop on request errors #2666

Merged

lukesteensen mentioned this pull request Jun 5, 2020

ServiceSink never updates request ID #2741

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(sinks): Refactor internal sinks and simplify #2111

chore(sinks): Refactor internal sinks and simplify #2111

LucioFranco commented Mar 20, 2020 •

edited

Loading

binarylogic commented Mar 20, 2020

LucioFranco commented Mar 20, 2020

LucioFranco commented Mar 24, 2020 •

edited

Loading

LucioFranco commented Mar 26, 2020

bruceg left a comment

bruceg Mar 26, 2020

LucioFranco Mar 26, 2020

Hoverbear Mar 30, 2020 •

edited

Loading

LucioFranco Mar 30, 2020

Hoverbear Mar 30, 2020

lukesteensen Mar 30, 2020

Hoverbear left a comment

lukesteensen left a comment

lukesteensen Mar 30, 2020

LucioFranco Mar 30, 2020

lukesteensen Mar 30, 2020

LucioFranco Mar 30, 2020

LucioFranco Mar 31, 2020

LucioFranco commented Mar 30, 2020

chore(sinks): Refactor internal sinks and simplify #2111

chore(sinks): Refactor internal sinks and simplify #2111

Conversation

LucioFranco commented Mar 20, 2020 • edited Loading

binarylogic commented Mar 20, 2020

LucioFranco commented Mar 20, 2020

LucioFranco commented Mar 24, 2020 • edited Loading

LucioFranco commented Mar 26, 2020

bruceg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hoverbear Mar 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hoverbear left a comment

Choose a reason for hiding this comment

lukesteensen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucioFranco commented Mar 30, 2020

LucioFranco commented Mar 20, 2020 •

edited

Loading

LucioFranco commented Mar 24, 2020 •

edited

Loading

Hoverbear Mar 30, 2020 •

edited

Loading