Aws logstream #27707

FrenchBen · 2016-10-25T00:41:14Z

- What I did
Added support for tags to the AWS log driver - The logstream flag will still over-write the tag, allowing for backwards compatibility. The tag parses the regular tag as seen in syslog, etc.

- How I did it
I updated the default of logStream from container ID to use the tag parsing default.

- How to verify it
make test (although I need to add some test for tags passed as flag)
make

- Description for the changelog
Update of AWS log driver to support tags. This enables support for things like --log-opt tag="{{.ImageName}}/{{.Name}}/{{.ID}}" as a tag for CloudWatch.

- A picture of a cute animal (not mandatory but encouraged)

cc @samuelkarp

samuelkarp · 2016-10-25T03:16:46Z

@FrenchBen Thanks for writing this and thanks for tagging me! At first glance this looks like #25633 and #27480. While many of the log drivers are using tag, I'd still lean against it for the reason I wrote here: #17747 (comment) (tags have a different meaning in the context of AWS).

FrenchBen · 2016-10-25T18:12:00Z

@samuelkarp Thanks for the quick review.
Keeping things simple and consistent with the existing tag functionality was my main focus, and as you mentioned many drivers currently use.
Creating an aws-specific tag, such as aws-tag to match the current options naming, would be the go-to if it needed to have the same meaning as the AWS tags in #17747 (comment).

IMHO

Add template support to awslog driver options like the fluentd log driver #27480 offers too lose of a PR where the log group is dynamic (against policy since the group needs to exist for logging to happen) and ignores unknown options.
Allow templating of AWS log stream name #25633 looks pretty good but seems to duplicate some of the template logic that currently exists in the tag. I do like the fact that it includes tests which I may re-use for this PR.

vdemeester · 2016-10-26T22:56:13Z

I do agree with @FrenchBen on the two other PRs. For the naming, same things, I would have liked to keep using tag (as other logging driver do) but I'm also okay with a aws specific one if needed — we would just have to change ParseLogTag to be able to pass which name to use.

Aside from the naming, design LGTM on this one 👼

samuelkarp

@FrenchBen I'm still not wild about tag since the primitive here is a log stream, but I think you're right that we could create an awslogs-tags option if necessary.

samuelkarp · 2016-10-26T23:44:18Z

daemon/logger/awslogs/cloudwatchlogs.go

@@ -350,6 +355,7 @@ func ValidateLogOpt(cfg map[string]string) error {
 		case logGroupKey:
 		case logStreamKey:
 		case regionKey:
+		case "tag":


Any reason this is not a constant? It would be useful to share the constant across all the log drivers that validate option names (maybe loggerutils.TagKey?).

@samuelkarp that's a good point… not sure why this (and labels/env) are not constant. But this could be done in a follow-up PR 👼

samuelkarp · 2016-10-26T23:46:07Z

daemon/logger/awslogs/cloudwatchlogs.go

@@ -88,7 +89,11 @@ func init() {
 // the EC2 Instance Metadata Service.
 func New(ctx logger.Context) (logger.Logger, error) {
 	logGroupName := ctx.Config[logGroupKey]
-	logStreamName := ctx.ContainerID
+	logStreamName, err := loggerutils.ParseLogTag(ctx, loggerutils.DefaultTemplate)


Can you make it a validation error to specify both tag and awslogs-stream?

I don't believe this should trigger an error - Specifying awslogs-stream with tag will have no error, as awslogs-stream is "king" and overwrites the tag specified.

This allows you to update the driver and use the tag specified when no logstream have been specified, but not see any change in currently running containers that have the --log-opt awslogs-stream="someApp" (keeps this driver backwards compatible)

Is the use-case for setting --log-opt tag="{{.Name}}/{{.ID}}" (or similar) on the daemon so that it provides the template by default? If so, can we get the docs updated as well?

@samuelkarp that's exactly right, the idea is to set a template on the daemon for all new containers.
I'll make sure to create a PR for the docs to show that tag is now allowed but awslogs-stream would overwrite the tag.

I just looked at loggerutils.DefaultTemplate and it's {{.ID}}, which would be a change in behavior from the existing driver. Can you use {{.FullID}} instead to keep the existing behavior?

@FrenchBen Can you change the template to {{.FullID}} by default so the driver retains its existing behavior?

samuelkarp · 2016-10-26T23:47:37Z

daemon/logger/awslogs/cloudwatchlogs_test.go

+	}
+	argument := <-mockClient.createLogStreamArgument
+
+	if *argument.LogStreamName != "test-container/container-ab" {


Is ParseLogTag enforcing a length limit here? The maximum length of the stream name is 512 characters.

Tag only over-writes the logStreamName; whatever restrictions were in place before, are still in place.
Essentially I went from taking the default for logStreamName := ctx.ContainerID to use the default of the tag (which is also ctx.ContainerID), but adds the flexibility of tag parsing as seen with all other log drivers.

What's shortening "container-abcdefghijklmnopqrstuvwxyz01234567890" to "container-ab"? I would have expected this assertion to be "test-container/container-abcdefghijklmnopqrstuvwxyz01234567890".

Per the docs:
https://docs.docker.com/engine/admin/logging/log_tags/
{{.ID}} The first 12 characters of the container id.
expr length "container-ab" = 12

Got it. It might be nicer to change the template used for the test to one that doesn't involve truncation (maybe {{.FullID}}) so it's more obvious what's going on?

This test came about to mimic the existing logging test found here:
https://github.com/docker/docker/blob/master/daemon/logger/loggerutils/log_tag_test.go#L21

I don't feel super strongly about this, but having to look in another location to see why it's truncated makes the test more difficult for someone unfamiliar with the codebase to read.

More than happy to update the PR to show the FullID if that helps getting an LGTM from you ;)

vdemeester · 2016-10-27T00:47:48Z

/me tentatively add this to the 1.13.0 milestone 😝

samuelkarp · 2016-10-28T00:04:57Z

daemon/logger/awslogs/cloudwatchlogs_test.go

@@ -719,6 +719,6 @@ func TestCreateTagSuccess(t *testing.T) {
 	argument := <-mockClient.createLogStreamArgument

 	if *argument.LogStreamName != "test-container/container-ab" {


Looks like it's going to fail now because the assertion didn't get updated.

Yup - make it fail, then show that it's working :)

FrenchBen · 2016-10-28T18:39:28Z

@samuelkarp all green! 💚

samuelkarp · 2016-10-28T22:12:16Z

@FrenchBen Are you planning to open a docs PR after this is merged?

FrenchBen · 2016-10-28T23:49:02Z

@samuelkarp absolutely!

samuelkarp · 2016-10-29T05:04:28Z

@FrenchBen Thanks for opening the docs PR! The only remaining feedback I have is about the default stream name when both awslogs-stream and tag are not specified (see this comment).

vdemeester

design LGTM 🐸

FrenchBen · 2016-10-31T20:13:38Z

Missed the comment - Let me update the PR to reflect that

samuelkarp

LGTM

thaJeztah · 2016-11-03T02:58:50Z

@FrenchBen can you squash your commits?

vdemeester

LGTM 🐸 once commit squashed 👼

FrenchBen · 2016-11-03T19:39:28Z

⚡️ @vdemeester

thaJeztah

LGTM

thanks for reviewing @samuelkarp!

cpuguy83 · 2016-11-03T20:37:57Z

@FrenchBen It'd be nice to get these into 1 commit.

samuelkarp · 2016-11-03T20:43:04Z

@thaJeztah Happy to help!

thaJeztah · 2016-11-03T20:45:09Z

oh! I thought it was squashed, darn. @FrenchBen ❤️

squashy

Signed-off-by: French Ben <[email protected]>

thaJeztah

LGTM

thaJeztah · 2016-11-03T23:15:59Z

all :green: merging!

ozbillwang · 2020-07-23T11:48:34Z

Could anyone explain how to use this new feature with awslogs-stream?

could you share the docker run command and docker version?

I did the same as --log-opt awslogs-stream="$HOSTNAME-{{.ID}}", but I didn't get the docker container id properly.

docker run --log-driver=awslogs --log-opt awslogs-region=ap-southeast-2
  --log-opt awslogs-create-group=true 
  --log-opt awslogs-group="hello-world"
  --log-opt awslogs-stream="$HOSTNAME-{{.ID}}"
  hello-world

Here is the log stream name I saw, it doesn't show container id of {{.ID}}

ip-10-0-2-123.ap-southeast-2.compute.internal-{{.ID}}

I did try these log_tags one by one, seems no one works.

https://docs.docker.com/config/containers/logging/log_tags/

{{.ID}} | The first 12 characters of the container ID.
{{.FullID}} | The full container ID.
{{.Name}} | The container name.
{{.ImageID}} | The first 12 characters of the container’s image ID.
{{.ImageFullID}} | The container’s full image ID.
{{.ImageName}} | The name of the image used by the container.
{{.DaemonName}} | The name of the docker program (docker).

samuelkarp · 2020-07-24T04:03:18Z

Use --log-opt tag, not --log-opt awslogs-stream. See the documentation.

ozbillwang · 2020-07-24T05:13:17Z

Thanks a lot, that works.

docker run --log-driver=awslogs --log-opt awslogs-region=ap-southeast-2
  --log-opt awslogs-create-group=true 
  --log-opt awslogs-group="hello-world"
  --log-opt tag="$HOSTNAME/{{.ID}}"
  hello-world

seems log-opt awslogs-stream has conflict with tag, they can't be used together. If used both, awslogs-stream gets the priority.

GordonTheTurtle added the status/0-triage label Oct 25, 2016

thaJeztah added status/1-design-review and removed status/0-triage labels Oct 25, 2016

samuelkarp reviewed Oct 27, 2016

View reviewed changes

vdemeester added this to the 1.13.0 milestone Oct 27, 2016

vdemeester self-assigned this Oct 27, 2016

samuelkarp reviewed Oct 28, 2016

View reviewed changes

FrenchBen force-pushed the aws-logstream branch from 2e7b5d6 to 01da0c6 Compare October 28, 2016 00:14

This was referenced Oct 29, 2016

Added details about tag support for AWS log driver docker/docs#378

Closed

Added details about tag support for AWS log driver docker/docs#380

Merged

vdemeester approved these changes Oct 29, 2016

View reviewed changes

samuelkarp approved these changes Oct 31, 2016

View reviewed changes

thaJeztah added status/2-code-review and removed status/1-design-review labels Nov 3, 2016

thaJeztah added impact/changelog impact/documentation labels Nov 3, 2016

vdemeester approved these changes Nov 3, 2016

View reviewed changes

FrenchBen force-pushed the aws-logstream branch from bfcdbc5 to 183a509 Compare November 3, 2016 19:37

thaJeztah previously approved these changes Nov 3, 2016

View reviewed changes

thaJeztah added status/4-merge and removed status/2-code-review labels Nov 3, 2016

Updated AWS logstream to understand tags

3661510

Signed-off-by: French Ben <[email protected]>

FrenchBen force-pushed the aws-logstream branch from 183a509 to 3661510 Compare November 3, 2016 20:49

This was referenced Nov 3, 2016

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

Closed

Allow templating of AWS log stream name #25633

Closed

thaJeztah approved these changes Nov 3, 2016

View reviewed changes

thaJeztah merged commit f424b1d into moby:master Nov 3, 2016

FrenchBen deleted the aws-logstream branch November 4, 2016 02:02

This was referenced Dec 12, 2016

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

Closed

awslogs driver support for variables using template markup in awslogs-stream #17747

Closed

This was referenced Jan 3, 2017

awslog driver not taking docker image name, ecs service name and task definition #29840

Closed

Unable to use tcp in --gelf-address #29871

Closed

		@@ -719,6 +719,6 @@ func TestCreateTagSuccess(t *testing.T) {
		argument := <-mockClient.createLogStreamArgument

		if *argument.LogStreamName != "test-container/container-ab" {

Aws logstream #27707

Aws logstream #27707

Conversation

FrenchBen commented Oct 25, 2016

samuelkarp commented Oct 25, 2016

FrenchBen commented Oct 25, 2016 • edited Loading

vdemeester commented Oct 26, 2016

samuelkarp 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

FrenchBen Oct 27, 2016 • 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

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

FrenchBen Oct 27, 2016 • 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

Choose a reason for hiding this comment

vdemeester commented Oct 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrenchBen commented Oct 28, 2016

samuelkarp commented Oct 28, 2016

FrenchBen commented Oct 28, 2016

samuelkarp commented Oct 29, 2016

vdemeester left a comment

Choose a reason for hiding this comment

FrenchBen commented Oct 31, 2016

samuelkarp left a comment

Choose a reason for hiding this comment

thaJeztah commented Nov 3, 2016

vdemeester left a comment

Choose a reason for hiding this comment

FrenchBen commented Nov 3, 2016

thaJeztah left a comment

Choose a reason for hiding this comment

cpuguy83 commented Nov 3, 2016

samuelkarp commented Nov 3, 2016

thaJeztah commented Nov 3, 2016

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah commented Nov 3, 2016

ozbillwang commented Jul 23, 2020 • edited Loading

samuelkarp commented Jul 24, 2020

ozbillwang commented Jul 24, 2020 • edited Loading

FrenchBen commented Oct 25, 2016 •

edited

Loading

FrenchBen Oct 27, 2016 •

edited

Loading

FrenchBen Oct 27, 2016 •

edited

Loading

ozbillwang commented Jul 23, 2020 •

edited

Loading

ozbillwang commented Jul 24, 2020 •

edited

Loading