feat(new sink): Initial `gcp_stackdriver_logging` sink implementation #1555

bruceg · 2020-01-20T23:20:13Z

Adds a sink to support GCP Stackdriver Logging

binarylogic

Looks like a good first pass, excited to get this merged. Just a few documentation comments I had.

.meta/sinks/gcp_stackdriver_logging.toml

binarylogic · 2020-01-20T23:41:51Z

.meta/sinks/gcp_stackdriver_logging.toml

+examples = ["vector-123456"]
+description = """\
+The billing account ID to which to publish logs.
+Exactly one of `billing_account_id`, `folder_id`, `log_id`, or `organization_id` must be set.\


To clarify, a user cannot supply multiple of these attributes, they must supply exactly one? If that's the case I wonder if it would make more sense to have a single polymorphic attribute:

[sinks.stackdriver] type = "gcp_stackdriver_logging" billing.type = "account" billing.id = "12345"

Just food for thought. I don't completely understand what these attributes are used for.

The log name is one of projects/[PROJECT_ID]/logs/[LOG_ID], folders/[FOLDER_ID]/logs[LOG_ID], etc. Exactly one of the four attributes must be supplied to construct the log name.

I see however that I made a mistake in which four they are, should include project_id instead of log_id.

binarylogic · 2020-01-20T23:43:14Z

.meta/sinks/gcp_stackdriver_logging.toml

+required = "true"
+examples = ["global", "gce_instance"]
+description = """\
+The monitored resource type. \


I'm not sure I completely understand what this is. Could you explain a little further? What does global mean in this context? Are there docs we could link to that further describe what this is?

Yeah, understanding this requires a bit of knowledge of how Stackdriver organizes logs. It was a bit confusing to me at first too. The docs are here: https://cloud.google.com/logging/docs/reference/v2/rest/v2/MonitoredResource

binarylogic · 2020-01-20T23:44:16Z

.meta/sinks/gcp_stackdriver_logging.toml

+common = false
+examples = ["vector-123456"]
+description = """\
+The log ID to which to publish logs.


Can we link to docs or help the user understand where they can obtain this ID? Or should it be common sense for a Stackdriver user?

It might be common sense, not sure. It is not an ID that is retrieved from GCP, rather it is something the user would generate based on what they are logging from. That is, you can specify anything and Stackdriver will file it away.

binarylogic · 2020-01-20T23:44:22Z

.meta/sinks/gcp_stackdriver_logging.toml

+common = false
+examples = ["vector-123456"]
+description = """\
+The folder ID to which to publish logs.


Can we link to docs or help the user understand where they can obtain this ID? Or should it be common sense for a Stackdriver user?

.meta/sinks/gcp_stackdriver_logging.toml

binarylogic · 2020-01-20T23:45:33Z

.meta/sinks/gcp_stackdriver_logging.toml

+request_retry_attempts = 9223372036854775807
+request_retry_initial_backoff_secs = 1
+request_retry_max_duration_secs = 10
+request_in_flight_limit = 5


Is this aligned with API limits or just an arbitrary limit?

Most are arbitrary, but the rate limit and batch size are from the API docs: https://cloud.google.com/logging/quotas#log-limits

.meta/sinks/gcp_stackdriver_logging.toml

LucioFranco

This looks great! I think the way you did the splice was really smart!

I'd like to see the two config changes before we merge.

Another thing, is maybe we want to create a src/sinks/gcp folder that contains the util file, the stackdriver and pubsub sink. What do you think?

src/sinks/gcp_stackdriver_logging.rs

LucioFranco · 2020-01-24T22:44:26Z

src/sinks/gcp_stackdriver_logging.rs

+                logs.pop(); // Strip the trailing comma
+
+                let mut body = wrapper.clone().into_bytes();
+                body.splice(wrapper_splice..wrapper_splice, logs);


This is a cool solution! I've been thinking all week how I want to accomplish this and I think this is as close to a good solution as well get.

I have some ideas of how to improve this but won't block this PR on it.

Frankly, I thought this was a huge hack, but if you like it, I'm happy. Best would of course be some kind of JSON serializer that could do incremental work, but it's not obvious how that might work in practice.

I am of course curious what could be done to improve on it.

Yeah, ive been mulling over this problem for a while since this is a major thing with making easy to write http sinks. I plan on seeing if I can clean up this hack next week. I think you have basically the gist of it. I think we could possibly write a custom body actually for http that could basically have slots for 3 slices so we don't have to do any double allocations like what is happening now. The other idea also is to use bytes as the "static" bytes and arc clone them rather then recopying the vec.

But I think for this right now, this is a good hack and its not messy either so I'm happy to keep this for now.

src/sinks/gcp_stackdriver_logging.rs

LucioFranco · 2020-01-24T23:19:28Z

I think this also could use a rebase against master.

bruceg · 2020-01-24T23:23:21Z

Another thing, is maybe we want to create a src/sinks/gcp folder that contains the util file, the stackdriver and pubsub sink. What do you think?

It would make some sense. If we're going to do that, we should probably do similar with the AWS too, which makes it a "code debt" kind of thing. At least it's a separate PR.

LucioFranco · 2020-01-24T23:26:13Z

@bruceg good idea! I opened an issue for aws and possibly datadog #1603. I still think we should do the change here for gcp since we are adding a new util file.

Signed-off-by: Bruce Guenter <bruce@timber.io>

LucioFranco

LGTM! Great work!

Signed-off-by: Bruce Guenter <bruce@timber.io>

Signed-off-by: binarylogic <bjohnson@binarylogic.com>

binarylogic · 2020-01-28T18:29:38Z

@bruceg I'm writing documentation on GCP authentication. Do you think it makes sense to support the GOOGLE_APPLICATION_CREDENTIALS environment variable? This seems to be a standard as noted in the GCP docs.

binarylogic · 2020-01-28T18:48:21Z

Also, it appears the api_key option is not documented? I'm working on fixing this.

Signed-off-by: binarylogic <bjohnson@binarylogic.com>

bruceg · 2020-01-28T20:21:06Z

I had thought GOOGLE_APPLICATION_CREDENTIALS might be support by the goauth crate we use, but there is no evidence of that in the source. It also affects validation of our config too, so I'll add that.

The api_key setting only works for certain APIs. It just so happens that the first GCP sink we implemented is one of those APIs (and it's not even listed there).

Signed-off-by: binarylogic <bjohnson@binarylogic.com>

Signed-off-by: Bruce Guenter <bruce@timber.io>

binarylogic

Nice work!

bruceg requested a review from binarylogic as a code owner January 20, 2020 23:20

bruceg force-pushed the gcp-logging-sink branch 2 times, most recently from 6f3640d to 6b8615c Compare January 20, 2020 23:26

binarylogic suggested changes Jan 20, 2020

View reviewed changes

binarylogic changed the title ~~feat(new sink): Google Cloud Platform Stackdriver Logging~~ feat(new sink): Initial gcp_stackdriver_logging sink implementation Jan 21, 2020

bruceg force-pushed the gcp-logging-sink branch from 044653a to e8560ae Compare January 21, 2020 23:12

binarylogic requested a review from LucioFranco January 22, 2020 05:28

binarylogic assigned LucioFranco Jan 22, 2020

bruceg requested a review from binarylogic January 22, 2020 22:14

LucioFranco suggested changes Jan 24, 2020

View reviewed changes

LucioFranco mentioned this pull request Jan 24, 2020

Possibly nest all "aws" sinks under an src/sinks/aws module #1603

Closed

Bruce Guenter added 2 commits January 24, 2020 17:27

Move GCP auth token generation into a module

4971a6d

Signed-off-by: Bruce Guenter <bruce@timber.io>

New sink, GCP stackdriver logging

1b84780

Signed-off-by: Bruce Guenter <bruce@timber.io>

bruceg force-pushed the gcp-logging-sink branch from 1b6c94c to 1b84780 Compare January 24, 2020 23:29

Move the GCP sinks into a subdirectory

ace53f9

Signed-off-by: Bruce Guenter <bruce@timber.io>

bruceg requested a review from LucioFranco January 24, 2020 23:43

LucioFranco approved these changes Jan 24, 2020

View reviewed changes

Run make generate

dac0d11

Signed-off-by: Bruce Guenter <bruce@timber.io>

bruceg assigned binarylogic and unassigned LucioFranco Jan 24, 2020

binarylogic added 3 commits January 28, 2020 13:23

Cleanup documentation language

a8a2b31

Signed-off-by: binarylogic <bjohnson@binarylogic.com>

Merge branch 'master' into gcp-logging-sink

71f6e6a

Run make generate

4114bad

Signed-off-by: binarylogic <bjohnson@binarylogic.com>

Update stackdriver docs

7e4fb50

Signed-off-by: binarylogic <bjohnson@binarylogic.com>

binarylogic and others added 3 commits January 28, 2020 15:36

Remove api_key from stackdriver docs

5bf1110

Fix generate service_providers bug

c33077d

Signed-off-by: binarylogic <bjohnson@binarylogic.com>

Add support for $GOOGLE_APPLICATION_CREDENTIALS for GCP credentials

009e51c

Signed-off-by: Bruce Guenter <bruce@timber.io>

binarylogic approved these changes Jan 28, 2020

View reviewed changes

bruceg merged commit 219b1a8 into master Jan 28, 2020

bruceg deleted the gcp-logging-sink branch January 28, 2020 21:31

bruceg mentioned this pull request Jan 28, 2020

New gcp_stackdriver_logs sink #572

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(new sink): Initial `gcp_stackdriver_logging` sink implementation #1555

feat(new sink): Initial `gcp_stackdriver_logging` sink implementation #1555

bruceg commented Jan 20, 2020

binarylogic left a comment

binarylogic Jan 20, 2020

bruceg Jan 21, 2020

binarylogic Jan 20, 2020

bruceg Jan 21, 2020

binarylogic Jan 20, 2020

bruceg Jan 21, 2020

binarylogic Jan 20, 2020

binarylogic Jan 20, 2020

bruceg Jan 21, 2020

LucioFranco left a comment

LucioFranco Jan 24, 2020

bruceg Jan 24, 2020

LucioFranco Jan 24, 2020

LucioFranco commented Jan 24, 2020

bruceg commented Jan 24, 2020

LucioFranco commented Jan 24, 2020

LucioFranco left a comment

binarylogic commented Jan 28, 2020

binarylogic commented Jan 28, 2020

bruceg commented Jan 28, 2020

binarylogic left a comment

feat(new sink): Initial gcp_stackdriver_logging sink implementation #1555

feat(new sink): Initial gcp_stackdriver_logging sink implementation #1555

Conversation

bruceg commented Jan 20, 2020

binarylogic 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

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

LucioFranco 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

LucioFranco commented Jan 24, 2020

bruceg commented Jan 24, 2020

LucioFranco commented Jan 24, 2020

LucioFranco left a comment

Choose a reason for hiding this comment

binarylogic commented Jan 28, 2020

binarylogic commented Jan 28, 2020

bruceg commented Jan 28, 2020

binarylogic left a comment

Choose a reason for hiding this comment

feat(new sink): Initial `gcp_stackdriver_logging` sink implementation #1555

feat(new sink): Initial `gcp_stackdriver_logging` sink implementation #1555