Add mixer adapter for Fluentd #2423

jeffmendoza · 2018-01-04T19:29:42Z

Supports logentry types and forwards on to a listening Fluentd daemon. Uses the forward input plugin on Fluentd. Uses the github.com/fluent/fluent-logger-golang/fluent client.

Screenshot of viewing logs in Kibana via Elasticsearch:

istio-testing · 2018-01-04T19:29:50Z

Hi @jeffmendoza. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

rshriram · 2018-01-04T19:36:51Z

This is cool!! Thanks for the contribution..

dcberg · 2018-01-04T19:56:28Z

Very nice contribution....thanks!

codecov · 2018-01-04T20:44:35Z

Codecov Report

❗ No coverage uploaded for pull request base (master@62dc933). Click here to learn what that means.
The diff coverage is 75.86%.

@@            Coverage Diff            @@
##             master    #2423   +/-   ##
=========================================
  Coverage          ?   76.35%           
=========================================
  Files             ?      218           
  Lines             ?    19453           
  Branches          ?        0           
=========================================
  Hits              ?    14853           
  Misses            ?     3786           
  Partials          ?      814

Flag	Coverage Δ
#broker	`75.36% <ø> (?)`
#mixer	`76.35% <75.86%> (?)`
#pilot	`78.55% <ø> (?)`
#security	`76.19% <ø> (?)`

Impacted Files	Coverage Δ
mixer/adapter/fluentd/fluentd.go	`75.86% <75.86%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 62dc933...4bb4cc7. Read the comment docs.

jeffmendoza · 2018-01-04T21:09:53Z

/assign @douglas-reid

istio-merge-robot · 2018-01-04T22:55:36Z

@jeffmendoza PR needs rebase

douglas-reid

Looks pretty good. Thanks for tackling. Left a few nits, mostly around comments.

douglas-reid · 2018-01-04T22:54:24Z

mixer/adapter/fluentd/config/config.proto

+option (gogoproto.equal_all) = false;
+option (gogoproto.gostring_all) = false;
+
+message Params {


Nit: Let's add some comments here on what the fluentd adapter is for and how one might use it. Currently, this is the source of generated website documentation (a la: https://istio.io/docs/reference/config/mixer/adapters/kubernetes.html). It'd be nice to have some verbose presentation of this adapter in the Reference section.

douglas-reid · 2018-01-04T22:55:38Z

mixer/adapter/inventory.yaml

@@ -10,3 +10,4 @@ servicecontrol: "istio.io/istio/mixer/adapter/servicecontrol"
 stackdriver: "istio.io/istio/mixer/adapter/stackdriver"
 statsd: "istio.io/istio/mixer/adapter/statsd"
 stdio: "istio.io/istio/mixer/adapter/stdio"
+fluentd: "istio.io/istio/mixer/adapter/fluentd"


question: what do you think about maintaining alphabetical ordering of this inventory?

I like it. Done

douglas-reid · 2018-01-04T22:56:24Z

mixer/adapter/fluentd/fluentd.go

+
+//go:generate $GOPATH/src/istio.io/istio/bin/mixer_codegen.sh -f mixer/adapter/fluentd/config/config.proto
+
+package fluentd


nit: similar to above, can you add a Package-level comment here explaining the role of the adapter, how it is used, and what templates it implements?

douglas-reid · 2018-01-04T22:59:52Z

mixer/adapter/fluentd/fluentd.go

+			delete(i.Variables, "tag")
+		}
+
+		err := h.logger.PostWithTime(tag.(string), i.Timestamp, i.Variables)


nit: collapse the following line, via something like:

if err := h.logger.PostWithTime(...); err != nil { return err }

douglas-reid · 2018-01-04T23:01:16Z

mixer/adapter/fluentd/fluentd.go

+		},
+		NewBuilder: func() adapter.HandlerBuilder { return &builder{} },
+		DefaultConfig: &config.Params{
+			Address: "localhost:24224",


should we move this up into a const and make it more obvious as to the default value?

douglas-reid · 2018-01-04T23:01:36Z

mixer/adapter/fluentd/config/config.proto

+
+message Params {
+    // Address that fluentd is listening, ex: 1.2.3.4:24224
+    string address = 1;


please add a comment about the default value assumed by code.

douglas-reid · 2018-01-04T23:02:58Z

mixer/adapter/fluentd/fluentd_test.go

+	}
+
+	for i, c := range cases {
+		t.Run(strconv.Itoa(i), func(t *testing.T) {


why not just name the tests for the Address being provided? then, upon failure, I know what the input was precisely.

Sounds good, let me figure this out.

douglas-reid · 2018-01-04T23:04:41Z

mixer/adapter/fluentd/fluentd_test.go

+	}
+
+	for i, c := range cases {
+		t.Run(strconv.Itoa(i), func(t *testing.T) {


as above, let's give these things reasonable names. the cases are complex enough that a readable name for each case would help others debug a bit.

douglas-reid · 2018-01-04T23:06:16Z

mixer/testdata/config/fluentd.yaml

+spec:
+  match: "true" # match for all requests
+  actions:
+   - handler: fdhandler.fluentd


at some point, we considered a convention of just 'handler.*' for handlers (in the case where there would only be one). not sure on the current state of that, but it may be worth considering.

douglas-reid · 2018-01-04T23:06:38Z

mixer/testdata/config/fluentd.yaml

+  name: fdhandler
+  namespace: istio-system
+spec:
+  address: "localhost:24224"


this is the default address, right?

Yes, I'd like to have is here in the test configs incase someone wants to copy it

douglas-reid

This is awesome! Can't wait to try it out.

douglas-reid · 2018-01-05T17:38:54Z

/lgtm

istio-merge-robot · 2018-01-05T17:48:37Z

/lgtm cancel //PR changed after LGTM, removing LGTM. @douglas-reid @jeffmendoza

jeffmendoza · 2018-01-05T17:50:36Z

/retest

istio-testing · 2018-01-05T17:50:52Z

@jeffmendoza: you can't request testing unless you are a istio member.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

douglas-reid · 2018-01-05T17:51:14Z

/lgtm

istio-merge-robot · 2018-01-05T18:29:19Z

/lgtm cancel //PR changed after LGTM, removing LGTM. @douglas-reid @jeffmendoza

jeffmendoza · 2018-01-05T19:30:21Z

Rebased again, keep getting merge conflicts as HEAD gets Gopkg.lock changes.

Supports logentry types. Connects to a listening fluentd daemon. Only config is the address of fluentd. Uses the "Name" of the logentry as the fluentd "tag", unless the logentry has a variable "tag". Adds a dependency on the "github.com/fluent/fluent-logger-golang/fluent" library.

douglas-reid · 2018-01-05T21:38:41Z

/lgtm

istio-merge-robot · 2018-01-05T21:39:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: douglas-reid

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [douglas-reid]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

istio-merge-robot · 2018-01-05T22:39:18Z

/test all [submit-queue is verifying that this PR is safe to merge]

istio-merge-robot · 2018-01-05T23:12:19Z

Automatic merge from submit-queue.

googlebot added the cla: yes label Jan 4, 2018

istio-testing added the needs-ok-to-test label Jan 4, 2018

jeffmendoza force-pushed the fluentd-adapter branch from 952a501 to 2a7194b Compare January 4, 2018 20:59

istio-testing assigned douglas-reid Jan 4, 2018

istio-merge-robot added the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 4, 2018

douglas-reid reviewed Jan 4, 2018

View reviewed changes

jeffmendoza force-pushed the fluentd-adapter branch from 5b40ef9 to 115896d Compare January 5, 2018 00:54

douglas-reid approved these changes Jan 5, 2018

View reviewed changes

istio-testing added the lgtm label Jan 5, 2018

istio-merge-robot added the approved label Jan 5, 2018

istio-merge-robot removed lgtm needs-rebase Indicates a PR needs to be rebased before being merged labels Jan 5, 2018

istio-testing added the lgtm label Jan 5, 2018

istio-merge-robot removed the lgtm label Jan 5, 2018

jeffmendoza force-pushed the fluentd-adapter branch from 3d4d7a7 to fc1c1e1 Compare January 5, 2018 19:12

jeffmendoza force-pushed the fluentd-adapter branch from fc1c1e1 to 4bb4cc7 Compare January 5, 2018 20:11

istio-testing added the lgtm label Jan 5, 2018

istio-merge-robot merged commit 5959336 into istio:master Jan 5, 2018

jeffmendoza deleted the fluentd-adapter branch January 5, 2018 23:18


		//go:generate $GOPATH/src/istio.io/istio/bin/mixer_codegen.sh -f mixer/adapter/fluentd/config/config.proto

		package fluentd

Add mixer adapter for Fluentd #2423

Add mixer adapter for Fluentd #2423

Uh oh!

Conversation

jeffmendoza commented Jan 4, 2018

Uh oh!

istio-testing commented Jan 4, 2018

Uh oh!

rshriram commented Jan 4, 2018

Uh oh!

dcberg commented Jan 4, 2018

Uh oh!

codecov bot commented Jan 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jeffmendoza commented Jan 4, 2018

Uh oh!

istio-merge-robot commented Jan 4, 2018

Uh oh!

douglas-reid left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

douglas-reid left a comment

Choose a reason for hiding this comment

Uh oh!

douglas-reid commented Jan 5, 2018

Uh oh!

istio-merge-robot commented Jan 5, 2018

Uh oh!

jeffmendoza commented Jan 5, 2018

Uh oh!

istio-testing commented Jan 5, 2018

Uh oh!

douglas-reid commented Jan 5, 2018

Uh oh!

istio-merge-robot commented Jan 5, 2018

Uh oh!

codecov bot commented Jan 4, 2018 •

edited

Loading