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

Fixed `raw` mode splunk logger #34520

Merged
merged 1 commit into from Sep 19, 2017

Conversation

Projects
None yet
5 participants
@fnoeding

fnoeding commented Aug 15, 2017

fixes #34749

- What I did
I tried to use the Splunk logger with the raw format without a tag. I noticed that my logs stopped appearing in Splunk, after an empty line was produced by my docker containers. Since the splunk log driver will repeatedly try to submit the invalid log event, no more logs will be accepted by Splunk.

Here's a minimal sample of my configuration:

docker run \
	--rm \
	-it \
	--log-driver splunk \
	--log-opt splunk-token=${SPLUNK_TOKEN} \
	--log-opt splunk-url=http://127.0.0.1:8088 \
	--log-opt splunk-format=raw \
	--log-opt tag="" \
	--hostname ${HOSTNAME} \
	alpine \
	/bin/sh -c "for i in \`seq 10\`; do echo \"hello world from docker \${i}\"; sleep 1; done; echo \"empty line will follow\"; echo \"\"; echo \"    \"; echo \"done\"; "

The first few echo commands will succeed, but the one with the empty or whitespace only lines will break the logging to Splunk.

- How I did it

First checked the behaviour of Splunk: Events with empty or whitespace-only event's are rejected by Splunk HEC. So I've added a check to the Splunk logger, that skips whitespace-only events in raw mode.

- How to verify it

See above. I've also extended the unit tests for the Splunk log driver

- Description for the changelog

Fixed raw mode Splunk logger when used without a tag

@fnoeding

This comment has been minimized.

Show comment
Hide comment
@fnoeding

fnoeding Aug 15, 2017

Failed checks are due to an unrelated error:

14:37:34 FAIL: docker_cli_search_test.go:47: DockerSuite.TestSearchCmdOptions
14:37:34 
14:37:34 docker_cli_search_test.go:72:
14:37:34     c.Assert(outSearchCmdOfficialSlice, checker.HasLen, 3) // 1 header, 1 line, 1 carriage return
14:37:34 ... obtained []string = []string{"NAME      DESCRIPTION   STARS     OFFICIAL   AUTOMATED", ""}
14:37:34 ... n int = 3
14:37:34 
14:37:34

Do you know if there's already a fix for this? If so, I can rebase.

fnoeding commented Aug 15, 2017

Failed checks are due to an unrelated error:

14:37:34 FAIL: docker_cli_search_test.go:47: DockerSuite.TestSearchCmdOptions
14:37:34 
14:37:34 docker_cli_search_test.go:72:
14:37:34     c.Assert(outSearchCmdOfficialSlice, checker.HasLen, 3) // 1 header, 1 line, 1 carriage return
14:37:34 ... obtained []string = []string{"NAME      DESCRIPTION   STARS     OFFICIAL   AUTOMATED", ""}
14:37:34 ... n int = 3
14:37:34 
14:37:34

Do you know if there's already a fix for this? If so, I can rebase.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Aug 15, 2017

Member

failing test looks like an issue with Docker Hub, or CI not able to reach docker hub; let me trigger CI again

Member

thaJeztah commented Aug 15, 2017

failing test looks like an issue with Docker Hub, or CI not able to reach docker hub; let me trigger CI again

@fnoeding

This comment has been minimized.

Show comment
Hide comment
@fnoeding

fnoeding Aug 17, 2017

@thaJeztah still the same issue. Can you retry again?

fnoeding commented Aug 17, 2017

@thaJeztah still the same issue. Can you retry again?

Florian Noeding
Fixed `raw` mode splunk logger
Splunk HEC does not accept log events with an empty string or a
whitespace-only string.

Signed-off-by: Florian Noeding <florian@noeding.com>
@fnoeding

This comment has been minimized.

Show comment
Hide comment
@fnoeding

fnoeding Aug 18, 2017

Build on z failed due to

10:59:55 FAIL: check_test.go:348: DockerSwarmSuite.TearDownTest
10:59:55 
10:59:55 unmount of /tmp/docker-execroot/d61562b5a35e9/netns failed: invalid argument
10:59:55 unmount of /tmp/docker-execroot/d61562b5a35e9/netns failed: no such file or directory
10:59:55 unmount of /tmp/docker-execroot/d68dcf869d700/netns failed: invalid argument
10:59:55 unmount of /tmp/docker-execroot/d68dcf869d700/netns failed: no such file or directory
10:59:55 unmount of /tmp/docker-execroot/df1308656c60e/netns failed: no such file or directory
10:59:55 unmount of /tmp/docker-execroot/d09af4e542c7d/netns failed: no such file or directory
10:59:55 check_test.go:353:
10:59:55     d.Stop(c)
10:59:55 daemon/daemon.go:392:
10:59:55     t.Fatalf("Error while stopping the daemon %s : %v", d.id, err)
10:59:55 ... Error: Error while stopping the daemon d4ea919fa4f84 : exit status 130
10:59:55 
10:59:55 
10:59:55 ----------------------------------------------------------------------
10:59:55 PANIC: docker_api_swarm_test.go:757: DockerSwarmSuite.TestAPISwarmRestartCluster

Retry again?

fnoeding commented Aug 18, 2017

Build on z failed due to

10:59:55 FAIL: check_test.go:348: DockerSwarmSuite.TearDownTest
10:59:55 
10:59:55 unmount of /tmp/docker-execroot/d61562b5a35e9/netns failed: invalid argument
10:59:55 unmount of /tmp/docker-execroot/d61562b5a35e9/netns failed: no such file or directory
10:59:55 unmount of /tmp/docker-execroot/d68dcf869d700/netns failed: invalid argument
10:59:55 unmount of /tmp/docker-execroot/d68dcf869d700/netns failed: no such file or directory
10:59:55 unmount of /tmp/docker-execroot/df1308656c60e/netns failed: no such file or directory
10:59:55 unmount of /tmp/docker-execroot/d09af4e542c7d/netns failed: no such file or directory
10:59:55 check_test.go:353:
10:59:55     d.Stop(c)
10:59:55 daemon/daemon.go:392:
10:59:55     t.Fatalf("Error while stopping the daemon %s : %v", d.id, err)
10:59:55 ... Error: Error while stopping the daemon d4ea919fa4f84 : exit status 130
10:59:55 
10:59:55 
10:59:55 ----------------------------------------------------------------------
10:59:55 PANIC: docker_api_swarm_test.go:757: DockerSwarmSuite.TestAPISwarmRestartCluster

Retry again?

@@ -363,6 +364,11 @@ func (l *splunkLoggerJSON) Log(msg *logger.Message) error {
}
func (l *splunkLoggerRaw) Log(msg *logger.Message) error {
// empty or whitespace-only messages are not accepted by HEC
if strings.TrimSpace(string(msg.Line)) == "" {

This comment has been minimized.

@thaJeztah

thaJeztah Aug 18, 2017

Member

one more thing I was thinking, as performance may be important for logging; should we check for real empty first? e.g.;

if len(msg.Line) == 0 || strings.TrimSpace(....

(sorry, I'm traveling, reading all notifications from my phone 😅)

@thaJeztah

thaJeztah Aug 18, 2017

Member

one more thing I was thinking, as performance may be important for logging; should we check for real empty first? e.g.;

if len(msg.Line) == 0 || strings.TrimSpace(....

(sorry, I'm traveling, reading all notifications from my phone 😅)

This comment has been minimized.

@fnoeding

fnoeding Aug 18, 2017

I believe performance at this place is not of sufficient importance to make the logic more complicated:

  • most lines will not be empty or whitespace only, so the common case is a line that will be logged
  • whenever a line is logged we write data to kernel space. A syscall is probably much more expensive than TrimSpace
  • but if you know of a better way of checking "string has at least one non-whitespace character" that would be a nice improvement
@fnoeding

fnoeding Aug 18, 2017

I believe performance at this place is not of sufficient importance to make the logic more complicated:

  • most lines will not be empty or whitespace only, so the common case is a line that will be logged
  • whenever a line is logged we write data to kernel space. A syscall is probably much more expensive than TrimSpace
  • but if you know of a better way of checking "string has at least one non-whitespace character" that would be a nice improvement

This comment has been minimized.

@fnoeding

fnoeding Aug 29, 2017

@thaJeztah do you have further feedback for me? And how can we get this merged soon?

@fnoeding

fnoeding Aug 29, 2017

@thaJeztah do you have further feedback for me? And how can we get this merged soon?

@fnoeding

This comment has been minimized.

Show comment
Hide comment
@fnoeding

fnoeding Aug 31, 2017

@tiborvass any further feedback to get this merged? :)

fnoeding commented Aug 31, 2017

@tiborvass any further feedback to get this merged? :)

@fnoeding

This comment has been minimized.

Show comment
Hide comment
@fnoeding

fnoeding Sep 12, 2017

@thaJeztah @tiborvass how can I help to get this merged?

fnoeding commented Sep 12, 2017

@thaJeztah @tiborvass how can I help to get this merged?

@thaJeztah

sorry for the delay; LGTM, thanks!

ping @cpuguy83 @vieux PTAL

@vdemeester

LGTM 🐮

@vdemeester vdemeester merged commit 5c57ca1 into moby:master Sep 19, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 36355 has succeeded
Details
janky Jenkins build Docker-PRs 44979 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 5367 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 16415 has succeeded
Details
z Jenkins build Docker-PRs-s390x 5182 has succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment