Skip to content
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

log driver - Interpolate fields into log tag #15384

Merged
merged 1 commit into from Sep 17, 2015

Conversation

phil-monroe
Copy link
Contributor

Fixes #15058

Substitute variables about the context of the container into the syslog tag. The following go templates are added:

  • {{.ID}} - The first 12 characters of the container id.
  • {{.FullID}} - The full id of the container.
  • {{.Name}} - The name of the container.
  • {{.ImageID}} - The first 12 characters of the image id used for the container.
  • {{.ImageFullID}} - The full id of the image used for the container.
  • {{.ImageName}} - The name of the image used for the container.

example:

docker run --log-driver=syslog --log-opt syslog-tag="{{.ImageName}}/{{.Name}}/{{.ID}}" --name foobar hello-world

output in syslog:

Aug  6 22:20:04 80fbe5afa9a9 docker/hello-world/foobar/c24f89499fce[952]: Hello from Docker.
Aug  6 22:20:04 80fbe5afa9a9 docker/hello-world/foobar/c24f89499fce[952]: This message shows that your installation appears to be working correctly.
Aug  6 22:20:04 80fbe5afa9a9 docker/hello-world/foobar/c24f89499fce[952]:
Aug  6 22:20:04 80fbe5afa9a9 docker/hello-world/foobar/c24f89499fce[952]: To generate this message, Docker took the following steps:
Aug  6 22:20:04 80fbe5afa9a9 docker/hello-world/foobar/c24f89499fce[952]:  1. The Docker client contacted the Docker daemon.
Aug  6 22:20:04 80fbe5afa9a9 docker/hello-world/foobar/c24f89499fce[952]:  2. The Docker daemon pulled the "hello-world" image from the Docker Hub.
Aug  6 22:20:04 80fbe5afa9a9 docker/hello-world/foobar/c24f89499fce[952]:     (Assuming it was not already locally available.)
Aug  6 22:20:04 80fbe5afa9a9 docker/hello-world/foobar/c24f89499fce[952]:  3. The Docker daemon created a new container from that image which runs the
Aug  6 22:20:04 80fbe5afa9a9 docker/hello-world/foobar/c24f89499fce[952]:     executable that produces the output you are currently reading.
Aug  6 22:20:04 80fbe5afa9a9 docker/hello-world/foobar/c24f89499fce[952]:  4. The Docker daemon streamed that output to the Docker client, which sent it
Aug  6 22:20:04 80fbe5afa9a9 docker/hello-world/foobar/c24f89499fce[952]:     to your terminal.
Aug  6 22:20:04 80fbe5afa9a9 docker/hello-world/foobar/c24f89499fce[952]:
Aug  6 22:20:04 80fbe5afa9a9 docker/hello-world/foobar/c24f89499fce[952]: To try something more ambitious, you can run an Ubuntu container with:
Aug  6 22:20:04 80fbe5afa9a9 docker/hello-world/foobar/c24f89499fce[952]:  $ docker run -it ubuntu bash
Aug  6 22:20:04 80fbe5afa9a9 docker/hello-world/foobar/c24f89499fce[952]:
Aug  6 22:20:04 80fbe5afa9a9 docker/hello-world/foobar/c24f89499fce[952]: For more examples and ideas, visit:
Aug  6 22:20:04 80fbe5afa9a9 docker/hello-world/foobar/c24f89499fce[952]:  http://docs.docker.com/userguide/

@GordonTheTurtle GordonTheTurtle added status/0-triage dco/no Automatically set by a bot when one of the commits lacks proper signature labels Aug 6, 2015
@phil-monroe phil-monroe force-pushed the 15058-include-name-in-syslog-tag branch from d63e5fa to ac7fbcf Compare August 6, 2015 23:13
@GordonTheTurtle GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Aug 6, 2015
@phil-monroe phil-monroe force-pushed the 15058-include-name-in-syslog-tag branch from ac7fbcf to 4156f3f Compare August 6, 2015 23:24
@icecrime
Copy link
Contributor

icecrime commented Aug 7, 2015

Cool, seems like a good idea (and code LGTM). @LK4D4 WDYT?

@phemmer
Copy link
Contributor

phemmer commented Aug 7, 2015

I think the idea is sound, but personally I would prefer go template formatting. We already have a few commands that use go templates, and I would prefer not to see another template format.

If the template had access to the ContainerJSON object, that'd be even better, as then the template would support the same fields as docker inspect. But this is probably too complex.

@ghost
Copy link

ghost commented Aug 7, 2015

There is an opened pull request to address the same issue for the fluentd driver, and it uses go templates.

@cpuguy83
Copy link
Member

cpuguy83 commented Aug 7, 2015

+1 for go-template if we do this.

@phil-monroe phil-monroe force-pushed the 15058-include-name-in-syslog-tag branch from 4156f3f to 0744010 Compare August 7, 2015 18:54
@phil-monroe
Copy link
Contributor Author

Updated to follow the go template convention and include documentation.

@icecrime
Copy link
Contributor

icecrime commented Aug 7, 2015

@phemmer Good call thanks. Moving to code review.

@@ -167,3 +178,31 @@ func parseFacility(facility string) (syslog.Priority, error) {

return syslog.Priority(0), errors.New("invalid syslog facility")
}

func parseTag(ctx logger.Context) (string, error) {
tag := ctx.Config["syslog-tag"]
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to move this (and setting the default (line 194)) outside of this function; I.e. pass a template and the "tag" and return the result. This would open up the possibility to re-use this for other log drivers at some point.

(Thinking out loud here :-))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So something like:

func parseTag(ctx logger.Context, tagTemplate string, defaultTagTemplate string) (string, error) {
    if tagTemplate == "" {
        tagTemplate = defaultTagTemplate
    }
    //...
}

and would be used like so:

tag, err := parseTag(ctx, ctx.Config["syslog-tag"], "{{.ID}}")

Copy link
Member

Choose a reason for hiding this comment

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

Yes, something along that line. I'm not really a code reviewer here 😊, but I expect that other log-drivers may want to use templating as well, so having something more reusable would be great.

Perhaps some other people have suggestions here (@LK4D4 ?)

Choose a reason for hiding this comment

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

+1 to something reusable.

@phil-monroe phil-monroe force-pushed the 15058-include-name-in-syslog-tag branch 2 times, most recently from c7ff302 to b0c4a05 Compare August 10, 2015 17:59
@phil-monroe
Copy link
Contributor Author

Updated to make the code hopefully more reusable.

@phil-monroe phil-monroe force-pushed the 15058-include-name-in-syslog-tag branch from b0c4a05 to ed2e260 Compare August 10, 2015 21:01
@phil-monroe
Copy link
Contributor Author

Just pushed an update to namespace the helper method as to not clutter logger.go and add some unit specs.

@phil-monroe
Copy link
Contributor Author

Do those build failures seem like they relate to this or should just be retried?

@thaJeztah
Copy link
Member

@phil-monroe I triggered a rebuild; let's see 👍

@phil-monroe
Copy link
Contributor Author

@thaJeztah seems like things passed.

@icecrime / @cpuguy83 / @phemmer / @eolamey - Any other comments for the code?

@cpuguy83
Copy link
Member

@phil-monroe Have you considered using the existing Context struct and adding methods to get the fields we want to support to that directly?

@mariussturm
Copy link
Contributor

This should also be consistent for all logging drivers which support tags, not only the syslog-driver.

@calavera
Copy link
Contributor

I agree with @mariussturm. @phil-monroe can you make changes in the other drivers too?

@phil-monroe
Copy link
Contributor Author

Sorry for the delay, I've been on vacation. I'll get the changes in here soon.

@mariussturm, @calavera - Sure thing.

@cpuguy83 - For some reason I thought the logger.Context was crucial to the container and was worried that someone could exploit the function capabilities of go templates to do something dangerous. On second look, I don't think that there should be any real security concern from giving raw access to the
logger.Context. Do you (or anyone) think that there could be a security issue with that? I agree that helper methods on that struct would be better than a separate context if there aren't any concerns there. I'm probably being overly sensitive here as I'm new to go and the docker codebase.

@phil-monroe
Copy link
Contributor Author

Updated the documentation. Let me know what you think.

The `syslog-tag` specifies a tag that identifies the container's syslog messages. By default,
the system uses the first 12 characters of the container id. To override this behavior, specify
a `syslog-tag` option
See the [log tag option](/reference/logging/log_tags/) documentation on how to format log tags.
Copy link
Member

Choose a reason for hiding this comment

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

I think we should give slightly more context to this link. Something like this;

By default, Docker uses the first 12 characters of the container ID to tag log messages.
Refer to the [log tag option documentation](/reference/logging/log_tags/) for customizing
the log tag format.

@thaJeztah
Copy link
Member

Added some suggestions for the docs; given that we're deprecating the driver specific "tag" options, can you add that to the deprecated features document. It should mention that the syslog-tag, gelf-tag and fluentd-tag log options are deprecated in docker 1.9, and will be removed in docker 1.11 (we try to keep deprecated options around for 2 releases)

Thanks @phil-monroe ! Great work altogether

ping @moxiegirl

@thaJeztah
Copy link
Member

Oh, and ping @albers because the completion scripts will need to be updated ❤️

@thaJeztah
Copy link
Member

Almost forgot; ping @sdurrheimer for zsh completions, once this is merged 👍

@albers
Copy link
Member

albers commented Aug 29, 2015

I'll add this to bash completion. Thanks for pinging me, @thaJeztah.

@sdurrheimer sdurrheimer mentioned this pull request Aug 29, 2015
3 tasks
@@ -0,0 +1,39 @@
<!--[metadata]>
Copy link
Contributor

Choose a reason for hiding this comment

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

@phil-monroe Thanks for the contribution! Nicely done. Can you add a link to this file in the reference/logging/index.md page? Some tweaks to this page, table works better for reference also I think better to show a simple use oftag first then use of the templates.

--> https://gist.github.com/moxiegirl/5d5fbd9bd282c3dce3bd

Log Tags

The tag log option specifies how to format a tag that identifies the
container's log messages. By default, the system uses the first 12 characters of
the container id. To override this behavior, specify a tag option:

docker run --log-driver=fluentd --log-opt fluentd-address=myhost.local:24224 --log-opt tag="mailer"

Docker supports some special template markup you can use when specifying a tag's value:

Markup Description
{{.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 identifier.
{{.ImageName}} The name of the image used by the container.

For example, specifying a --log-opt tag="{{.ImageName}}/{{.Name}}/{{.ID}}" value yields syslog log lines like:

Aug  7 18:33:19 HOSTNAME docker/hello-world/foobar/5790672ab6a0[9103]: Hello from Docker.

At startup time, the system sets the container_name field and {{.Name}} in
the tags. If you use docker rename to rename a container, the new name is not
reflected in the log messages. Instead, these messages continue to use the
original container name.

For advanced usage, the generated tag's use go
templates
and the container's logging
context
.

Note:The driver specific log options syslog-tag, fluentd-tag and
gelf-tag still work for backwards compatibility. However, going forward you
should standardize on using the generic tag log option instead.

@cpuguy83
Copy link
Member

Ping, any chance you can update the docs?

@moxiegirl
Copy link
Contributor

@cpuguy83 if necessary, I can carry. Just an FYI

@phil-monroe
Copy link
Contributor Author

Sorry for the delay! Crazy couple of weeks. I'll try and finish them off
today.

On Tuesday, September 15, 2015, moxiegirl notifications@github.com wrote:

@cpuguy83 https://github.com/cpuguy83 if necessary, I can carry. Just
an FYI


Reply to this email directly or view it on GitHub
#15384 (comment).

Phil Monroe

@cpuguy83
Copy link
Member

Thanks!

…g tag field

Signed-off-by: Philip Monroe <phil@philmonroe.com>
@phil-monroe phil-monroe force-pushed the 15058-include-name-in-syslog-tag branch from 3b12b99 to 3be7146 Compare September 16, 2015 22:20
@phil-monroe
Copy link
Contributor Author

Documentation updated. Sorry for the radio silence!

@moxiegirl
Copy link
Contributor

@phil-monroe Thanks! LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet