Allow templating of AWS log stream name #25633

jdnurmi · 2016-08-11T21:23:38Z

I ended up needing something similar to 17747 and drafted this guy up - it's a fairly straight-forward implementation, but let me know if I'm missing any style-wants/etc.

There's items it could probably use (strip ':'s from ImageId's comes to mind since they're forbidden by the CW logs API), but hopefully a good enough start for others as well.

jdnurmi · 2016-08-11T23:19:30Z

It would appear the win2lin test was broken for a bit this evening, if someone can retrigger that test, I believe it should pass.

thaJeztah · 2016-08-12T00:03:28Z

/cc @samuelkarp @cpuguy83

samuelkarp · 2016-08-13T00:59:47Z

daemon/logger/awslogs/cloudwatchlogs.go

 func New(ctx logger.Context) (logger.Logger, error) {
 	logGroupName := ctx.Config[logGroupKey]
 	logStreamName := ctx.ContainerID
 	if ctx.Config[logStreamKey] != "" {
 		logStreamName = ctx.Config[logStreamKey]
+	} else if ctx.Config[logStreamTemplateKey] != "" {
+		if t, err := template.New("t").Parse(ctx.Config[logStreamTemplateKey]); err == nil {


It would be good to extract this out to a separate function for readability.

samuelkarp · 2016-08-13T01:01:38Z

Thanks for submitting this!

Can you add some unit tests to exercise the new functionality you added? It would also be good to update the documentation (in docs/admin/logging/awslogs.md).

jdnurmi · 2016-08-22T21:11:29Z

I think I got everything after some fighting with the code vetter, let me know if I left anything to be desired.

samuelkarp · 2016-08-24T01:22:38Z

docs/admin/logging/awslogs.md

+# awslogs-stream-template
+
+Configure a [log stream](http://docs.aws.amazon.com/AmazonCloudWatch/latest/DeveloperGuide/WhatIsCloudWatchLogs.html)
+to a templated name.  The template is executed with a [logger.Context](http://docs.aws.amazon.com/AmazonCloudWatch/latest/DeveloperGuide/WhatIsCloudWatchLogs.html)


I think you have the wrong link for logger.Context.

indeed - will fix

cpuguy83 · 2016-08-26T18:41:27Z

Templating seems fairly generic enough to support in all drivers.
Otherwise I'm ok with the idea.

cpuguy83 · 2016-09-15T18:19:27Z

That is to say, +1. There isn't a need to implement all drivers here, but I don't think we need to namespace the option? WDYT?

thaJeztah · 2016-09-15T19:22:28Z

I think that makes sense; we should take care to properly document that it's currently only supported in this driver, but otherwise sgtm

thaJeztah · 2016-09-22T16:47:45Z

ping @jdnurmi can you address the review comments?

jdnurmi · 2016-09-28T12:42:49Z

Ok, I think I've got everything; Originally I'd thought that putting this on the Logger.Context directly might make sense, but I didn't want to muck with anything too deep unless there seemed like consensus.

It would be short work to do that if that's what people would like.

EDIT: And sorry for the delay!

samuelkarp

Changes look great! I'm going to ask our doc writer if he has any suggestions on the documentation, but otherwise it looks good.

samuelkarp · 2016-09-28T17:37:20Z

daemon/logger/awslogs/cloudwatchlogs.go

+			name = buff.String()
+		}
+		if err == nil && strings.IndexAny(name, ":*") >= 0 {
+			// Arguably, we could let AWS reject this later, but by then your container is


(*logStream).create() synchronously calls CreateLogStream which should reject invalid names. Since errors resulting from that call cause New() to return an error, the container should not actually start running. However, I don't object to local validity checks in the name like this.

nrdlngr

Thanks @samuelkarp for suggesting for doc review. Just a few doc suggestions for the option for consistency with the existing docs.

nrdlngr · 2016-09-28T19:46:37Z

docs/admin/logging/awslogs.md

+to a templated name.  The template is executed with a [logger.Context](https://godoc.org/github.com/docker/docker/daemon/logger#Context)
+as context, so awslogs-stream-template="{{ .ContainerID }}" would be equivelant to the default behavior.
+
+See the note above with regards to name uniqueness and performance.


It wouldn't hurt to copy the above note to here, to avoid ambiguity.

nrdlngr · 2016-09-28T20:03:31Z

docs/admin/logging/awslogs.md

@@ -63,6 +63,14 @@ specified, the container ID is used as the log stream.
 > at a time.  Using the same log stream for multiple containers concurrently
 > can cause reduced logging performance.

+# awslogs-stream-template
+
+Configure a [log stream](http://docs.aws.amazon.com/AmazonCloudWatch/latest/DeveloperGuide/WhatIsCloudWatchLogs.html)


You might try something like this:

Specify a template to automatically generate the log stream name to use. The template supports logger.Context, so awslogs-stream-template="{{ .ContainerID }}" would be equivalent to the default behavior.

@samuelkarp

- fixes moby#17747 - Uses the logging.Context for template context - Add tests, check for conflicting options, gofmt - Add documentation - golint ++ vs += - testing pedantry: 'text/template.ExecError composite literal uses unkeyed fields' in tests per @samuelkarp - Moved location - fixed up log message * ahem. and fix the test - refactor style (explict returns) to match rest of file per @nrdlngr - Documentation clarity improvements - Fix logging link - Remove spurious note Signed-off-by: James Nurmi <[email protected]>

jdnurmi · 2016-09-29T01:24:00Z

Note copied, text changed as specified - let me know if I've dropped any comments here.

thaJeztah · 2016-09-29T08:56:51Z

ping @dnephin @vdemeester PTAL

jdnurmi · 2016-10-05T01:10:26Z

fwiw, I believe the windows failure is just a fluke, if someone could retrigger it, I believe this will be passing again.

thaJeztah · 2016-10-21T17:55:35Z

ping @dnephin @vdemeester could you review this, in combination with #27480 to see which implementation is best, and what we should to to get these moving?

vdemeester · 2016-11-03T20:54:21Z

hey @jdnurmi thanks so much for your contribution, but we are going #27707 which is a similar implementation tested and more inline with others log drivers. It will land in 1.13.0 😉

frezbo · 2018-02-06T19:25:11Z

@vdemeester @thaJeztah this PR is actually superior over the tags option I could state multiple reasons for this:

consider a service that keeps on restarting, I get multiple log stream o/p if I use tags with the container id, it is not at all easy to go through each log stream and this clutters the logstream
Consider a service having multiple replicas, we have a use case that we want each replica of the service to log to its own logstream (always to the same logstream whether the container dies and gets recreated)
The current implementation does not provide insight into which node produced the logs (we label node and restrain services on to specific nodes). Having no insight into which node produced the logs is so cumbersome and hard to debug,

I believe the current aws log driver options does not provide any useful implementation for production deployment.
I would suggest that both the logstream and tag support be there. This is what I would like to see:
awslogs-stream: "{{.Node.Hostname}}-{{.Service.Name}}-{{.Task.Name}}-{{.Task.Slot}}"

The ability to fine tune the logstream
Hope you get the issue

GordonTheTurtle added the status/0-triage label Aug 11, 2016

thaJeztah added status/1-design-review and removed status/0-triage labels Aug 12, 2016

samuelkarp reviewed Aug 13, 2016
View reviewed changes

jdnurmi force-pushed the 17747-awslogs-stream-template branch 6 times, most recently from ccac0b4 to 0993704 Compare August 22, 2016 18:27

samuelkarp reviewed Aug 24, 2016
View reviewed changes

thaJeztah added status/2-code-review and removed status/1-design-review labels Sep 15, 2016

jdnurmi force-pushed the 17747-awslogs-stream-template branch from 0993704 to 675b057 Compare September 28, 2016 12:41

thaJeztah added the impact/documentation label Sep 28, 2016

jdnurmi force-pushed the 17747-awslogs-stream-template branch 2 times, most recently from 91c0e39 to 068d2cb Compare September 28, 2016 12:56

samuelkarp approved these changes Sep 28, 2016

View reviewed changes

nrdlngr suggested changes Sep 28, 2016

View reviewed changes

jdnurmi force-pushed the 17747-awslogs-stream-template branch from 068d2cb to 5ef3a5e Compare September 29, 2016 01:21

jdnurmi force-pushed the 17747-awslogs-stream-template branch from 5ef3a5e to 2dd50e3 Compare September 29, 2016 01:23

This was referenced Oct 20, 2016

awslogs: add support for selecting group/stream from label instead of name #27362

Closed

Add template support to awslog driver options like the fluentd log driver #27480

Closed

thaJeztah assigned vdemeester and dnephin Oct 21, 2016

samuelkarp mentioned this pull request Oct 25, 2016

Aws logstream #27707

Merged

thaJeztah mentioned this pull request Oct 25, 2016

Add docker image name:revision attribute to journald logging driver events #21497

Closed

vdemeester closed this Nov 3, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow templating of AWS log stream name #25633

Allow templating of AWS log stream name #25633

jdnurmi commented Aug 11, 2016

jdnurmi commented Aug 11, 2016

thaJeztah commented Aug 12, 2016

samuelkarp Aug 13, 2016

samuelkarp commented Aug 13, 2016

jdnurmi commented Aug 22, 2016

samuelkarp Aug 24, 2016

jdnurmi Aug 24, 2016

cpuguy83 commented Aug 26, 2016

cpuguy83 commented Sep 15, 2016

thaJeztah commented Sep 15, 2016

thaJeztah commented Sep 22, 2016

jdnurmi commented Sep 28, 2016 •

edited

Loading

samuelkarp left a comment

samuelkarp Sep 28, 2016

nrdlngr left a comment

nrdlngr Sep 28, 2016

nrdlngr Sep 28, 2016

jdnurmi commented Sep 29, 2016

thaJeztah commented Sep 29, 2016

jdnurmi commented Oct 5, 2016

thaJeztah commented Oct 21, 2016

vdemeester commented Nov 3, 2016

frezbo commented Feb 6, 2018 •

edited

Loading

Allow templating of AWS log stream name #25633

Allow templating of AWS log stream name #25633

Conversation

jdnurmi commented Aug 11, 2016

jdnurmi commented Aug 11, 2016

thaJeztah commented Aug 12, 2016

samuelkarp Aug 13, 2016

Choose a reason for hiding this comment

samuelkarp commented Aug 13, 2016

jdnurmi commented Aug 22, 2016

samuelkarp Aug 24, 2016

Choose a reason for hiding this comment

jdnurmi Aug 24, 2016

Choose a reason for hiding this comment

cpuguy83 commented Aug 26, 2016

cpuguy83 commented Sep 15, 2016

thaJeztah commented Sep 15, 2016

thaJeztah commented Sep 22, 2016

jdnurmi commented Sep 28, 2016 • edited Loading

samuelkarp left a comment

Choose a reason for hiding this comment

samuelkarp Sep 28, 2016

Choose a reason for hiding this comment

nrdlngr left a comment

Choose a reason for hiding this comment

nrdlngr Sep 28, 2016

Choose a reason for hiding this comment

nrdlngr Sep 28, 2016

Choose a reason for hiding this comment

jdnurmi commented Sep 29, 2016

thaJeztah commented Sep 29, 2016

jdnurmi commented Oct 5, 2016

thaJeztah commented Oct 21, 2016

vdemeester commented Nov 3, 2016

frezbo commented Feb 6, 2018 • edited Loading

jdnurmi commented Sep 28, 2016 •

edited

Loading

frezbo commented Feb 6, 2018 •

edited

Loading