Skip to content

Added support for write-timeout support to fluentd plugin. #35716

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.

Sign up for GitHub

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

Closed
imumesh18 wants to merge 3 commits into from
Closed

Added support for write-timeout support to fluentd plugin. #35716

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0b5e38a
Updated dependency with version tag
imumesh18 Dec 6, 2017
7233a50
Added support for write timeout for fluentd plugin
imumesh18 Dec 6, 2017
989a19e
Addressed comments!
imumesh18 Dec 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions daemon/logger/fluentd/fluentd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,11 @@ const (
defaultRetryWait = 1000
defaultMaxRetries = math.MaxInt32

// Write() will time out after 20s
defaultWriteTimeout = time.Duration(20 * time.Second)

addressKey = "fluentd-address"
writeTimeoutKey = "fluentd-write-timeout"
bufferLimitKey = "fluentd-buffer-limit"
retryWaitKey = "fluentd-retry-wait"
maxRetriesKey = "fluentd-max-retries"
Expand Down Expand Up @@ -84,6 +88,18 @@ func New(info logger.Info) (logger.Logger, error) {
return nil, err
}

writeTimeout := defaultWriteTimeout
i, _ := strconv.Atoi(info.Config[writeTimeoutKey])
if info.Config[writeTimeoutKey] != "" && i >= 0 {
wtd, err := time.ParseDuration(info.Config[writeTimeoutKey])
Copy link
Copy Markdown
Contributor

@anusha-ragunathan anusha-ragunathan Apr 12, 2018

Choose a reason for hiding this comment

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

You can directly assign to writeTimeout

if err != nil {
return nil, err
}
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Dec 6, 2017

Choose a reason for hiding this comment

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

Should we check for negative durations and produce an error?

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Dec 6, 2017

Choose a reason for hiding this comment

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

Also, we need to check what status-code is returned by the API in case of an invalid value (haven't checked yet) (make sure it doesn't return a 5xx error)

Copy link
Copy Markdown
Contributor

@anusha-ragunathan anusha-ragunathan Dec 7, 2017

Choose a reason for hiding this comment

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

We should also check for extremely long timeout values to prevent daemon waiting on the write.

Copy link
Copy Markdown
Contributor Author

@imumesh18 imumesh18 Dec 9, 2017

Choose a reason for hiding this comment

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

So @anusha-ragunathan how long do you thing it should wait. I was thinking of something in minutes.

Copy link
Copy Markdown
Contributor Author

@imumesh18 imumesh18 Dec 9, 2017

Choose a reason for hiding this comment

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

@thaJeztah Actually I am completely new to these things so I am kind of learning how can I handle server error(5xx). It will take time but I will push the changes by next week. Actually I am bit busy with my college exams 🙈

Copy link
Copy Markdown
Contributor

@boaz0 boaz0 Dec 11, 2017

Choose a reason for hiding this comment

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

@dungeonmaster18 I think logger has ring buffer for this case. @cpuguy83

Copy link
Copy Markdown
Contributor Author

@imumesh18 imumesh18 Dec 11, 2017

Choose a reason for hiding this comment

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

As far I know ring buffer is nothing but the circular buffer with fixed size. Correct me if I am wrong @ripcurld0

Copy link
Copy Markdown
Contributor

@anusha-ragunathan anusha-ragunathan Dec 11, 2017

Choose a reason for hiding this comment

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

@dungeonmaster18 : regarding setting an upper limit on the timeout, lets skip that. Users should be aware of what they are setting for the timeout value. It's hard to predict a good max value. Also, I checked other timeouts. Other than disallowing negative values, there's no other check.

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Dec 12, 2017

Choose a reason for hiding this comment

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

For the status-code; The API uses this code https://github.com/moby/moby/blob/master/api/server/httputils/errors.go#L33-L73 (and code from the errdefs package: https://github.com/moby/moby/tree/master/api/errdefs) to convert errors from the daemon into the correct HTTP status codes. You can try creating an API request with an incorrect value for the timeout, and check what status code is returned.

writeTimeout = wtd
} else if i < 0 {
return nil, fmt.Errorf("Invalid Parameter: write-timeout must be a positive integer")
}

bufferLimit := defaultBufferLimit
if info.Config[bufferLimitKey] != "" {
bl64, err := units.RAMInBytes(info.Config[bufferLimitKey])
Expand Down Expand Up @@ -130,6 +146,7 @@ func New(info logger.Info) (logger.Logger, error) {
FluentHost: loc.host,
FluentNetwork: loc.protocol,
FluentSocketPath: loc.path,
WriteTimeout: writeTimeout,
BufferLimit: bufferLimit,
RetryWait: retryWait,
MaxRetry: maxRetries,
Expand Down Expand Up @@ -189,6 +206,7 @@ func ValidateLogOpt(cfg map[string]string) error {
case "tag":
case addressKey:
case bufferLimitKey:
case writeTimeoutKey:
case retryWaitKey:
case maxRetriesKey:
case asyncConnectKey:
Expand Down
4 changes: 2 additions & 2 deletions vendor.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,8 @@ github.com/Graylog2/go-gelf v2

github.com/fluent/fluent-logger-golang v1.3.0
# fluent-logger-golang deps
github.com/philhofer/fwd 98c11a7a6ec829d672b03833c3d69a7fae1ca972
github.com/tinylib/msgp 3b556c64540842d4f82967be066a7f7fffc3adad
github.com/philhofer/fwd v1.0.0
github.com/tinylib/msgp v1.0.2

# fsnotify
github.com/fsnotify/fsnotify 4da3e2cfbabc9f751898f250b49f2439785783a1
Expand Down
4 changes: 4 additions & 0 deletions vendor/github.com/philhofer/fwd/reader.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 1 addition & 2 deletions vendor/github.com/tinylib/msgp/msgp/unsafe.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.