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

Add logging driver for Google Cloud Logging #18766

Merged
merged 2 commits into from Mar 2, 2016

Conversation

Projects
None yet
@mikedanese
Contributor

mikedanese commented Dec 18, 2015

@mikedanese

This comment has been minimized.

Show comment
Hide comment
@mikedanese

mikedanese Dec 30, 2015

Contributor

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

Contributor

mikedanese commented Dec 30, 2015

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

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 30, 2015

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.

Member

thaJeztah commented Dec 30, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@mikedanese

mikedanese Dec 31, 2015

Contributor

Thanks @thaJeztah, no worries.

Contributor

mikedanese commented Dec 31, 2015

Thanks @thaJeztah, no worries.

@mikedanese

This comment has been minimized.

Show comment
Hide comment
@mikedanese

mikedanese Jan 13, 2016

Contributor

@thaJeztah Is anyone able to review this yet?

Contributor

mikedanese commented Jan 13, 2016

@thaJeztah Is anyone able to review this yet?

@mikedanese

This comment has been minimized.

Show comment
Hide comment
@mikedanese

mikedanese Jan 24, 2016

Contributor

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

Contributor

mikedanese commented Jan 24, 2016

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

@icecrime

This comment has been minimized.

Show comment
Hide comment
@icecrime

icecrime Jan 26, 2016

Contributor

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

LGTM, WDYT @LK4D4?

Contributor

icecrime commented Jan 26, 2016

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

LGTM, WDYT @LK4D4?

@thaJeztah thaJeztah added this to the 1.11.0 milestone Jan 26, 2016

@icecrime

This comment has been minimized.

Show comment
Hide comment
Contributor

icecrime commented Feb 2, 2016

Show outdated Hide outdated daemon/logger/gcplogs/gcplogging.go
}
// we start dropping logs at 10,000 log lines per second.
c.Overflow = func(_ *logging.Client, _ logging.Entry) error {

This comment has been minimized.

@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?

@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?

This comment has been minimized.

@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 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.

This comment has been minimized.

@mikedanese

mikedanese Feb 5, 2016

Contributor

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

@mikedanese

mikedanese Feb 5, 2016

Contributor

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

This comment has been minimized.

@LK4D4

LK4D4 Feb 5, 2016

Contributor

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

@LK4D4

LK4D4 Feb 5, 2016

Contributor

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

@mikedanese

This comment has been minimized.

Show comment
Hide comment
@mikedanese

mikedanese Feb 6, 2016

Contributor

@icecrime @LK4D4 I've updated the comment.

Contributor

mikedanese commented Feb 6, 2016

@icecrime @LK4D4 I've updated the comment.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Feb 9, 2016

Contributor

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

Contributor

cpuguy83 commented Feb 9, 2016

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

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Feb 9, 2016

Contributor

@cpuguy83 questions to @icecrime :)

Contributor

LK4D4 commented Feb 9, 2016

@cpuguy83 questions to @icecrime :)

@mikedanese

This comment has been minimized.

Show comment
Hide comment
@mikedanese

mikedanese Feb 9, 2016

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.

Contributor

mikedanese commented Feb 9, 2016

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

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 9, 2016

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

Member

thaJeztah commented Feb 9, 2016

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

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Feb 9, 2016

Contributor

Code changes LGTM

Contributor

calavera commented Feb 9, 2016

Code changes LGTM

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Feb 10, 2016

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 :(

Contributor

cpuguy83 commented Feb 10, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 15, 2016

Member

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

Member

thaJeztah commented Feb 15, 2016

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

Show outdated Hide outdated docs/admin/logging/gcplogs.md
@@ -0,0 +1,63 @@
<!--[metadata]>
+++
aliases = ["/engine/reference/logging/gcplogs/"]

This comment has been minimized.

@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 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

Show outdated Hide outdated docs/admin/logging/gcplogs.md
# Google Cloud Logging driver
The Google Cloud Logging driver sends container logs to [Google Cloud
Logging](https://cloud.google.com/logging/docs/).

This comment has been minimized.

@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 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 😄)

| Option | Required | Description |
|-----------------------------|----------|---------------------------------------------------------------------------------------------------------------------------------------------|
| `gcp-project` | optional | Which GCP project to log to. Defaults to discovering this value from the GCE metadata service. |

This comment has been minimized.

@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?

@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?

This comment has been minimized.

@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.

@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.

@@ -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. |

This comment has been minimized.

@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

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

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 18, 2016

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

Member

thaJeztah commented Feb 18, 2016

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

Show outdated Hide outdated contrib/completion/bash/docker
@@ -394,6 +394,7 @@ __docker_complete_isolation() {
__docker_complete_log_drivers() {
COMPREPLY=( $( compgen -W "
gcplogs

This comment has been minimized.

@albers

albers Feb 18, 2016

Member

please insert in alpabetical order

@albers

albers Feb 18, 2016

Member

please insert in alpabetical order

Show outdated Hide outdated 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"

This comment has been minimized.

@albers

albers Feb 18, 2016

Member

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

@albers

albers Feb 18, 2016

Member

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

This comment has been minimized.

@albers

albers Feb 18, 2016

Member

@mikedanese Two more things need to be done:

@thaJeztah thanks for the ping.

@albers

albers Feb 18, 2016

Member

@mikedanese Two more things need to be done:

@thaJeztah thanks for the ping.

Show outdated Hide outdated 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")

This comment has been minimized.

@sdurrheimer

sdurrheimer Feb 18, 2016

Contributor

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

@sdurrheimer

sdurrheimer Feb 18, 2016

Contributor

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

Show outdated Hide outdated 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

This comment has been minimized.

@sdurrheimer

sdurrheimer Feb 18, 2016

Contributor

nit: same here alphabetical order

@sdurrheimer

sdurrheimer Feb 18, 2016

Contributor

nit: same here alphabetical order

@mikedanese

This comment has been minimized.

Show comment
Hide comment
@mikedanese

mikedanese Feb 22, 2016

Contributor

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

Contributor

mikedanese commented Feb 22, 2016

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

@albers

This comment has been minimized.

Show comment
Hide comment
@albers

albers Feb 23, 2016

Member

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

Member

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

This comment has been minimized.

Show comment
Hide comment
@albers

albers Feb 23, 2016

Member

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

Member

albers commented Feb 23, 2016

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

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 23, 2016

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

Member

thaJeztah commented Feb 23, 2016

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

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>
@mikedanese

This comment has been minimized.

Show comment
Hide comment
@mikedanese

mikedanese Feb 24, 2016

Contributor

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

Done.

Contributor

mikedanese commented Feb 24, 2016

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

Done.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 24, 2016

Member

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

Member

thaJeztah commented Feb 24, 2016

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

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 29, 2016

Member

ping @mikedanese can you update your PR?

Member

thaJeztah commented Feb 29, 2016

ping @mikedanese can you update your PR?

daemon/logger: Add logging driver for Google Cloud Logging
Signed-off-by: Mike Danese <mikedanese@google.com>
@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Mar 1, 2016

Contributor

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

Contributor

calavera commented Mar 1, 2016

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

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 1, 2016

Member

Thanks @mikedanese sorry missed you updated the PR

tested the docs and LGTM!

Member

thaJeztah commented Mar 1, 2016

Thanks @mikedanese sorry missed you updated the PR

tested the docs and LGTM!

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 1, 2016

Member

restarted windowsTP4

Member

thaJeztah commented Mar 1, 2016

restarted windowsTP4

@mikedanese

This comment has been minimized.

Show comment
Hide comment
@mikedanese

mikedanese Mar 1, 2016

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!

Contributor

mikedanese commented Mar 1, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 2, 2016

Member

Oh, it's green now, merging!

Member

thaJeztah commented Mar 2, 2016

Oh, it's green now, merging!

thaJeztah added a commit that referenced this pull request Mar 2, 2016

Merge pull request #18766 from mikedanese/gcplogs
Add logging driver for Google Cloud Logging

@thaJeztah thaJeztah merged commit 3c4d093 into moby: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