Add logging driver for Google Cloud Logging #18766

Merged
merged 2 commits into from Mar 2, 2016

Projects

None yet
@mikedanese
Contributor
@mikedanese mikedanese closed this Dec 18, 2015
@mikedanese mikedanese reopened this Dec 18, 2015
@mikedanese
Contributor

@vdemeester how do i get a design review on this patch set?

@thaJeztah
Member

@mikedanese apologies for the delay, I'll see if there's a maintainer to review soon, but that may be next week due to New Year, and some maintainers having some days off for the holidays and new year.

@mikedanese
Contributor

Thanks @thaJeztah, no worries.

@mikedanese
Contributor

@thaJeztah Is anyone able to review this yet?

@mikedanese
Contributor

@thaJeztah @vdemeester is there any recommendation on how I can move this forward?

@icecrime
Member

That's a lot of dependencies.. ;-)

LGTM, WDYT @LK4D4?

@thaJeztah thaJeztah added this to the 1.11.0 milestone Jan 26, 2016
@icecrime
Member
icecrime commented Feb 2, 2016
@LK4D4 LK4D4 and 1 other commented on an outdated diff Feb 3, 2016
daemon/logger/gcplogs/gcplogging.go
+ }
+
+ if ctx.Config[logCmdKey] == "true" {
+ l.container.Command = ctx.Command()
+ }
+
+ if onGCE {
+ l.instance = &instanceInfo{
+ Zone: zone,
+ Name: instanceName,
+ ID: instanceID,
+ }
+ }
+
+ // we start dropping logs at 10,000 log lines per second.
+ c.Overflow = func(_ *logging.Client, _ logging.Entry) error {
@LK4D4
LK4D4 Feb 3, 2016 Contributor

I don't fully understand what does it mean. Do we log errors only once in 1000 dropped messages? What comment means?

@mikedanese
mikedanese Feb 3, 2016 Contributor

The logger "overflows" at a rate of 10,000 logs per second (but could be configured higher or lower) and this overflow func is called. I'm looking for a way to surface the error to the user. Currently it logs an error once every 1000 messages. If i log every message it spams /var/log/docker.log. I could log at most once every 5 seconds or I could ignore these logs.

@mikedanese
mikedanese Feb 5, 2016 Contributor

@LK4D4 Would you prefer I change this to work someway else?

@LK4D4
LK4D4 Feb 5, 2016 Contributor

No, it's ok, but better to put your answer in comment.

@mikedanese
Contributor

@icecrime @LK4D4 I've updated the comment.

@cpuguy83
Contributor
cpuguy83 commented Feb 9, 2016

I'm honestly not keen on adding ~12K LOC to support this.

@LK4D4
Contributor
LK4D4 commented Feb 9, 2016

@cpuguy83 questions to @icecrime :)

@mikedanese
Contributor

The added vendor dependency footprint is of the same order of magnitude as #15495. The added dependencies are self contained so they shouldn't conflict with other vendored dependencies. The vendored code is well tested. The code added to docker core is actually quite small (~180 lines). This seems preferable to reimplementing the log shipper in docker core.

@thaJeztah
Member

Yup, I don't like the +12K LOC, but I don't think there's much we can do until we have a plugin system in place for logging drivers.

So, I think we should move this forward, unless there's issues with the changes in docker/docker

@calavera
Contributor
calavera commented Feb 9, 2016

Code changes LGTM

@cpuguy83
Contributor

@mikedanese I had the same reservations about the other one :(
Unfortunately we don't have a good way to add logging drivers without vendoring-in so this'll have to do :(

@thaJeztah
Member

ping @mikedanese can you update the documentation, so that we can move this forward? Thanks!

@GordonTheTurtle GordonTheTurtle added dco/no and removed dco/no labels Feb 18, 2016
@thaJeztah thaJeztah commented on an outdated diff Feb 18, 2016
docs/admin/logging/gcplogs.md
@@ -0,0 +1,63 @@
+<!--[metadata]>
++++
+aliases = ["/engine/reference/logging/gcplogs/"]
@thaJeztah
thaJeztah Feb 18, 2016 Member

you can remove this line, because this is a new page to the documentation, so we don't need a redirect/alias for an old location

@thaJeztah thaJeztah commented on an outdated diff Feb 18, 2016
docs/admin/logging/gcplogs.md
+<!--[metadata]>
++++
+aliases = ["/engine/reference/logging/gcplogs/"]
+title = "Google Cloud Logging driver"
+description = "Describes how to use the Google Cloud Logging driver."
+keywords = ["gcplogs, google, docker, logging, driver"]
+[menu.main]
+parent = "smn_logging"
+weight = 2
++++
+<![end-metadata]-->
+
+# Google Cloud Logging driver
+
+The Google Cloud Logging driver sends container logs to [Google Cloud
+Logging](https://cloud.google.com/logging/docs/).
@thaJeztah
thaJeztah Feb 18, 2016 Member

Can you make this a HTML link with a target="_blank"? Overall, we try to open external links in a new tab/window so that readers can more easily continue reading the docs where they left off. i.e.;

<a href="https://cloud.google.com/logging/docs/" target="_blank">Google Cloud Logging</a>

(yeah, it's not as nice as a plain markdown link, but it does the job ๐Ÿ˜„)

@thaJeztah thaJeztah commented on the diff Feb 18, 2016
docs/admin/logging/gcplogs.md
+You can set the logging driver for a specific container by using the
+`--log-driver` option to `docker run`:
+
+ docker run --log-driver=gcplogs ...
+
+This log driver does not implement a reader so it is incompatible with
+`docker logs`.
+
+## gcplogs options
+
+You can use the `--log-opt NAME=VALUE` flag to specify these additional Google
+Cloud Logging driver options:
+
+| Option | Required | Description |
+|-----------------------------|----------|---------------------------------------------------------------------------------------------------------------------------------------------|
+| `gcp-project` | optional | Which GCP project to log to. Defaults to discovering this value from the GCE metadata service. |
@thaJeztah
thaJeztah Feb 18, 2016 Member

Defaults to discovering this value from the GCE metadata

I infer from this that this log-driver can only be used when running docker on GCE; should we mention that somewhere in the description of the driver?

@mikedanese
mikedanese Feb 22, 2016 Contributor

This is infact not the case but was not well documented. I clarified this in the # Usage section of the documentation.

@thaJeztah thaJeztah commented on the diff Feb 18, 2016
docs/admin/logging/overview.md
@@ -27,6 +27,7 @@ container's logging driver. The following options are supported:
| `awslogs` | Amazon CloudWatch Logs logging driver for Docker. Writes log messages to Amazon CloudWatch Logs. |
| `splunk` | Splunk logging driver for Docker. Writes log messages to `splunk` using HTTP Event Collector. |
| `etwlogs` | ETW logging driver for Docker on Windows. Writes log messages as ETW events. |
+| `gcplogs` | Google Cloud Logging driver for Docker. Writes log messages to Google Cloud Logging. |
@thaJeztah
thaJeztah Feb 18, 2016 Member

Same here; if only available when running on GCE, we may want to mention that (in this table, and the description below)

(ignore if that's not the case ๐Ÿ˜‡ )

@thaJeztah
Member

Thanks @mikedanese, couple of nits, but overall looking good.

I saw you also updated the completion scripts (w00t!);

ping @albers @sdurrheimer can you have a quick peek at the completion scripts, they're in this commit; mikedanese@1ca6a99

@albers albers commented on an outdated diff Feb 18, 2016
contrib/completion/bash/docker
@@ -394,6 +394,7 @@ __docker_complete_isolation() {
__docker_complete_log_drivers() {
COMPREPLY=( $( compgen -W "
+ gcplogs
@albers
albers Feb 18, 2016 Contributor

please insert in alpabetical order

@albers albers commented on an outdated diff Feb 18, 2016
contrib/completion/bash/docker
@@ -407,6 +408,7 @@ __docker_complete_log_drivers() {
__docker_complete_log_options() {
# see docs/reference/logging/index.md
+ local gcplogs_options="gcp-project labels env log-cmd"
@albers
albers Feb 18, 2016 Contributor

same here: please insert in alphabetical order, with the options alphabetically sorted.
Thanks for caring for the completions!

@albers
albers Feb 18, 2016 Contributor

@mikedanese Two more things need to be done:

@thaJeztah thanks for the ping.

@sdurrheimer sdurrheimer commented on an outdated diff Feb 18, 2016
contrib/completion/zsh/_docker
@@ -199,6 +199,7 @@ __docker_get_log_options() {
local log_driver=${opt_args[--log-driver]:-"all"}
local -a awslogs_options fluentd_options gelf_options journald_options json_file_options syslog_options splunk_options
+ gcplogs_options=("gcp-project" "labels" "env" "log-cmd")
@sdurrheimer
sdurrheimer Feb 18, 2016 Contributor

nit: like for the bash completion, please keep a alphabetical order.

@sdurrheimer sdurrheimer commented on an outdated diff Feb 18, 2016
contrib/completion/zsh/_docker
@@ -207,6 +208,7 @@ __docker_get_log_options() {
syslog_options=("syslog-address" "syslog-tls-ca-cert" "syslog-tls-cert" "syslog-tls-key" "syslog-tls-skip-verify" "syslog-facility" "tag")
splunk_options=("env" "labels" "splunk-caname" "splunk-capath" "splunk-index" "splunk-insecureskipverify" "splunk-source" "splunk-sourcetype" "splunk-token" "splunk-url" "tag")
+ [[ $log_driver = (gcplogs|all) ]] && _describe -t gcplogs-options "gcplogs options" gcplogs_options "$@" && ret=0
@sdurrheimer
sdurrheimer Feb 18, 2016 Contributor

nit: same here alphabetical order

@mikedanese
Contributor

Thanks for the review @thaJeztah @albers @sdurrheimer. I've append a commit that addresses the comments.

@albers
Contributor
albers commented Feb 23, 2016

The log-cmd option seems to be driver specific, so maybe it should be renamed to gcp-log-cmd.

@albers
Contributor
albers commented Feb 23, 2016

bash completion is perfect, thanks very much! LGTM (IANAM)

@thaJeztah
Member

The log-cmd option seems to be driver specific, so maybe it should be renamed to tcp-log-cmd.

Good suggestion if it's specific to this driver

I'll check the docs later

@mikedanese mikedanese vendor: add dependencies of gcplogs driver
The added dependencies are:
* golang.org/x/oauth2
* google.golang.org/api
* google.golang.org/cloud

Signed-off-by: Mike Danese <mikedanese@google.com>
123f220
@mikedanese
Contributor

The log-cmd option seems to be driver specific, so maybe it should be renamed to tcp-log-cmd.

Done.

@thaJeztah
Member

Thanks @mikedanese, last couple of nits, but we're almost there

@thaJeztah
Member

ping @mikedanese can you update your PR?

@mikedanese mikedanese daemon/logger: Add logging driver for Google Cloud Logging
Signed-off-by: Mike Danese <mikedanese@google.com>
ed1b9fa
@calavera
Contributor
calavera commented Mar 1, 2016

@thaJeztah docs update LGTM, can you take a look?

@thaJeztah
Member

Thanks @mikedanese sorry missed you updated the PR

tested the docs and LGTM!

@thaJeztah
Member

restarted windowsTP4

@mikedanese
Contributor

@thaJeztah Just sent it out this morning. I was going to wait for ci to go green before pinging you :) Thanks for the review!

@thaJeztah
Member

Oh, it's green now, merging!

@thaJeztah thaJeztah merged commit 3c4d093 into docker:master Mar 2, 2016

9 checks passed

docker/dco-signed All commits signed
Details
documentation success 2 tests run, 0 skipped, 0 failed.
Details
experimental Jenkins build Docker-PRs-experimental 15645 has succeeded
Details
gccgo Jenkins build Docker-PRs-gccgo 2543 has succeeded
Details
janky Jenkins build Docker-PRs 24434 has succeeded
Details
userns Jenkins build Docker-PRs-userns 6785 has succeeded
Details
vendor Jenkins build Docker-PRs-vendor 306 has succeeded
Details
win2lin Jenkins build Docker-PRs-Win2Lin 22549 has succeeded
Details
windowsTP4 Jenkins build Docker-PRs-WoW-TP4 1871 has succeeded
Details
@mikedanese mikedanese deleted the mikedanese:gcplogs branch Mar 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment