Better bundle config and error management #111
Conversation
james-bebbington
force-pushed
the
bundle
branch
from
46b820c
to
099ee28
Compare
| const defaultBundleDelayThreshold = 2 * time.Second | ||
| const defaultBundleCountThreshold = 50 | ||
| const bundleByteThresholdMultiplier = 300 |
There was a problem hiding this comment.
Do you remember what was the reason for these multipliers?
At the very least threshold must always be less or equal to the limit, I suppose? Do we need to validate this?
There was a problem hiding this comment.
Maybe we should just reuse the original defaults rather than defining our own?
https://github.com/googleapis/google-api-go-client/blob/2addc8164fd163f25eedca5869d464d8ef4b8b9b/support/bundler/bundler.go#L27-L30
There was a problem hiding this comment.
I'm not sure. I believe Steven did some benchmarking to determine what values worked best, so I am keen to leave these defaults that are probably better tuned for our use case than the bundler library defaults.
I don't know why he used multipliers for bundleByteThreshold / bundleByteLimit though. I just changed these to be more explicit (and removed the bundle byte limit).
| b.BundleByteThreshold = b.BundleCountThreshold * bundleByteThresholdMultiplier | ||
| b.BundleByteLimit = b.BundleCountThreshold * bundleByteLimitMultiplier | ||
|
|
||
| if o.BundleByteThreshold > 0 { |
There was a problem hiding this comment.
Doesn't block this PR, but should we return errors for invalid configuration instead of silently using the default?
There was a problem hiding this comment.
We should chat about that. I think for something like it we should do our best to ignore config errors and limp along not making your app worse.
However, this would be an undiscoverable config error, which should be fixed in some fashion...
There was a problem hiding this comment.
We could just pass these parameters as is to bundler.NewBundler and let it handle zero values and other validation.
There was a problem hiding this comment.
Doesn't look the bundler library does any validation of its own. I think in all their use cases, negative values will behave the same as zero which is arguably an idiomatic way to handle things in Go, e.g. They use DelayThreshold in time.AfterFunc which ends up hitting this condition in the Go source: https://github.com/golang/go/blob/5f3354d1bf2e6a61e4b9e1e31ee04b99dfe7de35/src/time/sleep.go#L29
Still, I'd be happy to make this return errors instead, but would do so in a follow up PR
There was a problem hiding this comment.
Or maybe that makes more sense to be something that is done by the code that reads config, e.g. in the Collector
There was a problem hiding this comment.
Sorry for not responding sooner. I think the important part here is from a user perspective, there is a correctable notification of failures and values aren't silently converted in a way that's confusing.
Likely that means work in whatever reads the config and passes to this function.
| b.BundleByteThreshold = b.BundleCountThreshold * bundleByteThresholdMultiplier | ||
| b.BundleByteLimit = b.BundleCountThreshold * bundleByteLimitMultiplier | ||
|
|
||
| if o.BundleByteThreshold > 0 { |
There was a problem hiding this comment.
We should chat about that. I think for something like it we should do our best to ignore config errors and limp along not making your app worse.
However, this would be an undiscoverable config error, which should be fixed in some fashion...
james-bebbington
force-pushed
the
bundle
branch
from
099ee28
to
4753bae
Compare
Changes:
Tests: