Add a filter plugin add_insert_ids. #246

qingling128 · 2018-08-28T02:33:22Z

No description provided.

bmoyles0117 · 2018-08-28T14:03:33Z

README.rdoc

+of your Fluentd configuration file, for example:
+
+    <filter **>
+      type add_insert_ids


Do we need a prefixed @?

bmoyles0117 · 2018-08-28T14:38:25Z

test/plugin/test_filter_add_insert_ids.rb

+      expected_insert_id = record[DEFAULT_INSERT_ID_KEY] if index == 0
+      assert_equal expected_insert_id, record[DEFAULT_INSERT_ID_KEY],
+                   "Index #{index} failed."
+      expected_insert_id = expected_insert_id.next


What do you think about checking to see if this is a unique value instead of mimicking the implementation?

What's a better alternative? I've been contemplating this and ended up adding the following line:

assert_true record[DEFAULT_INSERT_ID_KEY] < expected_insert_id, "Index #{index} failed. #{record[DEFAULT_INSERT_ID_KEY]}" \ " < #{expected_insert_id} is false."

Are you suggesting something like hashing all the insertIds and make sure we have 1000 unique ones?

Yes, hashing or counting a set of distinct values SGTM

Added a check against 1000 unique insertIds.

bmoyles0117 · 2018-08-28T14:39:26Z

test/plugin/test_filter_add_insert_ids.rb

+      record = event[2]
+      assert_true record.is_a?(Hash), "Index #{index} failed. Log record" \
+                  ' should be a hash.'
+      assert_equal index, record['id'], "Index #{index} failed."


This line seems to be testing the test itself, do we really need it?

This line makes sure the log entries come in order. On top of this we check the insertId is growing. It's meaningful IMO.

Can we reflect this in the error message?

bmoyles0117 · 2018-08-28T14:41:43Z

lib/fluent/plugin/filter_add_insert_ids.rb

+      def filter(tag, time, record)
+        # Only generate and add an insertId field If the record is a hash and
+        # the insert ID field is not already set.
+        if record.is_a?(Hash) && !record.key?(@insert_id_key)


Should we check for empty values as well?

is_a? checks that already.

2.4.3 :004 > ''.is_a?(Hash) => false 2.4.3 :005 > nil.is_a?(Hash) => false

Synced offline, checking the actual value within the record to make sure it's not blank. It might be over-optimizing, if the user really wants blank, that would be strange, but maybe it's intentional? I'll let you make the judgement call here as to whether or not blank values are an important scenario to auto-generate ids.

Added a check against an empty string.

qingling128

PTAL

qingling128 · 2018-08-28T19:55:25Z

README.rdoc

+of your Fluentd configuration file, for example:
+
+    <filter **>
+      type add_insert_ids


qingling128 · 2018-08-28T21:29:31Z

lib/fluent/plugin/filter_add_insert_ids.rb

+      def filter(tag, time, record)
+        # Only generate and add an insertId field If the record is a hash and
+        # the insert ID field is not already set.
+        if record.is_a?(Hash) && !record.key?(@insert_id_key)


is_a? checks that already.

2.4.3 :004 > ''.is_a?(Hash) => false 2.4.3 :005 > nil.is_a?(Hash) => false

qingling128 · 2018-08-28T21:31:52Z

test/plugin/test_filter_add_insert_ids.rb

+      record = event[2]
+      assert_true record.is_a?(Hash), "Index #{index} failed. Log record" \
+                  ' should be a hash.'
+      assert_equal index, record['id'], "Index #{index} failed."


This line makes sure the log entries come in order. On top of this we check the insertId is growing. It's meaningful IMO.

qingling128 · 2018-08-28T21:33:24Z

test/plugin/test_filter_add_insert_ids.rb

+      expected_insert_id = record[DEFAULT_INSERT_ID_KEY] if index == 0
+      assert_equal expected_insert_id, record[DEFAULT_INSERT_ID_KEY],
+                   "Index #{index} failed."
+      expected_insert_id = expected_insert_id.next


What's a better alternative? I've been contemplating this and ended up adding the following line:

assert_true record[DEFAULT_INSERT_ID_KEY] < expected_insert_id, "Index #{index} failed. #{record[DEFAULT_INSERT_ID_KEY]}" \ " < #{expected_insert_id} is false."

Are you suggesting something like hashing all the insertIds and make sure we have 1000 unique ones?

davidbtucker · 2018-08-28T21:42:15Z

README.rdoc

-which sends logs to the
-{Stackdriver Logging API}[https://cloud.google.com/logging/docs/api/].
+fluent-plugin-google-cloud gem includes two plugins:
+1. An {filter plugin for fluentd}[http://docs.fluentd.org/articles/filter-plugin-overview]


davidbtucker · 2018-08-28T21:46:13Z

README.rdoc

@@ -23,22 +23,31 @@ will also install and configure the gem.

 == Configuration

+To embed insertIds into log entries, specify <code>type add_insert_ids</code>
+in a {match clause}[http://docs.fluentd.org/articles/config-file#2-ldquomatchrdquo-tell-fluentd-what-to-do]


Should this be {filter clause}[https://docs.fluentd.org/v1.0/articles/config-file#(3)-%E2%80%9Cfilter%E2%80%9D:-event-processing-pipeline]?

davidbtucker · 2018-08-28T21:50:43Z

lib/fluent/plugin/filter_add_insert_ids.rb

+        # The default field name of insertIds in the log entry.
+        DEFAULT_INSERT_ID_KEY = 'logging.googleapis.com/insertId'.freeze
+        # The character size of the insertIds. This matches the setup in the
+        # Stackdriver Logging backend.


Can we link to any documentation about this value?

I tried. I did not see it anywhere in the public sites. This size was taken from the implementation code. In fact, that's just the default size the backend will generate. It's not a "requirement".

davidbtucker · 2018-08-28T21:52:56Z

lib/fluent/plugin/filter_add_insert_ids.rb

+        @log.info "Started the add_insert_ids plugin with #{@insert_id_key}" \
+                  ' as the insert ID key.'
+        @insert_id = generate_insert_id
+        @log.info "Initiated the insert ID key at #{@insert_id}."


Initiated -> Initialized?
Also: at -> to

davidbtucker · 2018-08-28T21:53:54Z

lib/fluent/plugin/filter_add_insert_ids.rb

+
+      # rubocop:disable Style/UnusedMethodArgument
+      def filter(tag, time, record)
+        # Only generate and add an insertId field If the record is a hash and


lib/fluent/plugin/filter_add_insert_ids.rb

davidbtucker · 2018-08-28T22:00:50Z

test/plugin/test_filter_add_insert_ids.rb

+    Fluent::Test.setup
+  end
+
+  def test_configure_insert_id_key


It might be clearer to inline the values and split this into 2 tests:

def test_configure_insert_id_key_with_default d = create_driver('') assert_equal DEFAULT_INSERT_ID_KEY, d.instance.insert_id_key def test_configure_insert_id_key_with_custom d = create_driver('insert_id_key custom_insert_id_key') assert_equal 'custom_insert_id_key', d.instance.insert_id_key

The rest of our unit tests mostly follow the test data provider pattern.

https://github.com/GoogleCloudPlatform/fluent-plugin-google-cloud/blob/master/test/plugin/base_test.rb#L65
https://github.com/GoogleCloudPlatform/fluent-plugin-google-cloud/blob/master/test/plugin/base_test.rb#L107

It's debatable which pattern is better. Let's discuss it and reach some consensus for future unit tests before introducing a stand-alone pattern. For this PR, I'll leave it as is.

If we do decide to switch, maybe this is a nice FixIt item to improve our tests' readability.

You're right, let's defer.

igorpeshansky · 2018-08-29T03:05:02Z

fluent-plugin-google-cloud.gemspec

  gem.homepage      =
    'https://github.com/GoogleCloudPlatform/fluent-plugin-google-cloud'
  gem.license       = 'Apache-2.0'
  gem.version       = '0.6.23'
-  gem.authors       = ['Ling Huang', 'Igor Peshansky']
+  gem.authors       = ['Stackdriver Agent Team']


Stackdriver Agents Team

igorpeshansky · 2018-08-29T03:05:45Z

fluent-plugin-google-cloud.gemspec

-   store them in Google Cloud Storage and/or BigQuery.
-   This is an official Google Ruby gem.
+   Fluentd plugins for the Stackdriver Logging API, which will make logs
+   viewable in the Developer Console's log viewer and can optionally store them


s/Developer Console's log viewer/Stackdriver Logs Viewer/

igorpeshansky · 2018-08-29T03:06:09Z

fluent-plugin-google-cloud.gemspec

-   This is an official Google Ruby gem.
+   Fluentd plugins for the Stackdriver Logging API, which will make logs
+   viewable in the Developer Console's log viewer and can optionally store them
+   in Google Cloud Storage and/or BigQuery. This is an official Google Ruby gem.


Let's move This is an official Google Ruby gem. back to its own line.

igorpeshansky · 2018-08-29T03:06:39Z

fluent-plugin-google-cloud.gemspec

 eos
-  gem.summary       = 'fluentd output plugin for the Stackdriver Logging API'
+  gem.summary       = 'fluentd plugins for the Stackdriver Logging API'


Why are we not releasing the insertId plugin as a separate gem? Expediency?

Expediency is one reason. Also the way it's used today is tied to out_google_cloud. The original proposal is to make it generic and not specific to Stackdriver (it makes more sense to create a new Github repo and go through launch process if that's the case).

There was a discussion thread in the design doc. We were debating it and ended up going with this approach.

igorpeshansky · 2018-08-29T03:08:13Z

lib/fluent/plugin/filter_add_insert_ids.rb

+# limitations under the License.
+
+require 'fluent/plugin/filter'
+require 'thread'


What is this needed for? It doesn't seem to be used in the code.

Oops. That's some residue. Originally I thought I need to implement some thread safety protection. But after digging into Fluentd's code, it's not necessary because each filter plugin in the configuration will have one instance only.

igorpeshansky · 2018-08-29T03:20:10Z

test/plugin/test_filter_add_insert_ids.rb

+  include Fluent::Plugin::AddInsertIdsFilter::ConfigConstants
+
+  CUSTOM_INSERT_ID_KEY = 'custom_insert_id_key'.freeze
+  APPLICATION_DEFAULT_CONFIG = ''.freeze


Do we want to keep the two configs next to each other?

LOL I'm evil and i made them break up. Changed.

igorpeshansky · 2018-08-29T03:20:19Z

test/plugin/test_filter_add_insert_ids.rb

+    Fluent::Test.setup
+  end
+
+  def test_configure_insert_id_key


igorpeshansky · 2018-08-29T03:22:00Z

test/plugin/test_filter_add_insert_ids.rb

+                   ' include 3 elements: tag, time and record.'
+      record = event[2]
+      assert_true record.is_a?(Hash), "Index #{index} failed. Log record" \
+                  ' should be a hash.'


Do you want to include what the record actually was in the error?

igorpeshansky · 2018-08-29T03:22:13Z

test/plugin/test_filter_add_insert_ids.rb

+    expected_insert_id = nil
+    filtered_events.each_with_index do |event, index|
+      assert_equal 3, event.size, "Index #{index} failed. Log event should" \
+                   ' include 3 elements: tag, time and record.'


Do you want to include the actual size in the error?

The actual size will be included in the error already. The

"Index #{index} failed. Log event should" \ ' include 3 elements: tag, time and record.'

does not overwrite the original not equal message. It only adds additional information.

igorpeshansky · 2018-08-29T03:22:52Z

test/plugin/test_filter_add_insert_ids.rb

+      record = event[2]
+      assert_true record.is_a?(Hash), "Index #{index} failed. Log record" \
+                  ' should be a hash.'
+      assert_equal index, record['id'], "Index #{index} failed."


Can we reflect this in the error message?

qingling128

Updated. PTAL

qingling128 · 2018-08-29T04:25:53Z

test/plugin/test_filter_add_insert_ids.rb

+      expected_insert_id = record[DEFAULT_INSERT_ID_KEY] if index == 0
+      assert_equal expected_insert_id, record[DEFAULT_INSERT_ID_KEY],
+                   "Index #{index} failed."
+      expected_insert_id = expected_insert_id.next


Added a check against 1000 unique insertIds.

qingling128 · 2018-08-29T04:26:15Z

README.rdoc

-which sends logs to the
-{Stackdriver Logging API}[https://cloud.google.com/logging/docs/api/].
+fluent-plugin-google-cloud gem includes two plugins:
+1. An {filter plugin for fluentd}[http://docs.fluentd.org/articles/filter-plugin-overview]


qingling128 · 2018-08-29T04:27:18Z

README.rdoc

@@ -23,22 +23,31 @@ will also install and configure the gem.

 == Configuration

+To embed insertIds into log entries, specify <code>type add_insert_ids</code>
+in a {match clause}[http://docs.fluentd.org/articles/config-file#2-ldquomatchrdquo-tell-fluentd-what-to-do]


qingling128 · 2018-08-29T04:28:49Z

fluent-plugin-google-cloud.gemspec

-   store them in Google Cloud Storage and/or BigQuery.
-   This is an official Google Ruby gem.
+   Fluentd plugins for the Stackdriver Logging API, which will make logs
+   viewable in the Developer Console's log viewer and can optionally store them


qingling128 · 2018-08-29T04:29:08Z

fluent-plugin-google-cloud.gemspec

-   This is an official Google Ruby gem.
+   Fluentd plugins for the Stackdriver Logging API, which will make logs
+   viewable in the Developer Console's log viewer and can optionally store them
+   in Google Cloud Storage and/or BigQuery. This is an official Google Ruby gem.


qingling128 · 2018-08-29T04:48:31Z

test/plugin/test_filter_add_insert_ids.rb

+  include Fluent::Plugin::AddInsertIdsFilter::ConfigConstants
+
+  CUSTOM_INSERT_ID_KEY = 'custom_insert_id_key'.freeze
+  APPLICATION_DEFAULT_CONFIG = ''.freeze


LOL I'm evil and i made them break up. Changed.

qingling128 · 2018-08-29T04:53:34Z

test/plugin/test_filter_add_insert_ids.rb

+    Fluent::Test.setup
+  end
+
+  def test_configure_insert_id_key


The rest of our unit tests mostly follow the test data provider pattern.

https://github.com/GoogleCloudPlatform/fluent-plugin-google-cloud/blob/master/test/plugin/base_test.rb#L65
https://github.com/GoogleCloudPlatform/fluent-plugin-google-cloud/blob/master/test/plugin/base_test.rb#L107

It's debatable which pattern is better. Let's discuss it and reach some consensus for future unit tests before introducing a stand-alone pattern. For this PR, I'll leave it as is.

If we do decide to switch, maybe this is a nice FixIt item to improve our tests' readability.

qingling128 · 2018-08-29T04:54:48Z

test/plugin/test_filter_add_insert_ids.rb

+    expected_insert_id = nil
+    filtered_events.each_with_index do |event, index|
+      assert_equal 3, event.size, "Index #{index} failed. Log event should" \
+                   ' include 3 elements: tag, time and record.'


The actual size will be included in the error already. The

"Index #{index} failed. Log event should" \ ' include 3 elements: tag, time and record.'

does not overwrite the original not equal message. It only adds additional information.

qingling128 · 2018-08-29T04:55:27Z

test/plugin/test_filter_add_insert_ids.rb

+                   ' include 3 elements: tag, time and record.'
+      record = event[2]
+      assert_true record.is_a?(Hash), "Index #{index} failed. Log record" \
+                  ' should be a hash.'


qingling128 · 2018-08-29T04:56:18Z

test/plugin/test_filter_add_insert_ids.rb

+      record = event[2]
+      assert_true record.is_a?(Hash), "Index #{index} failed. Log record" \
+                  ' should be a hash.'
+      assert_equal index, record['id'], "Index #{index} failed."


bmoyles0117 · 2018-08-29T13:59:25Z

README.rdoc

@@ -23,22 +23,31 @@ will also install and configure the gem.

 == Configuration

+To embed insertIds into log entries, specify <code>type add_insert_ids</code>


nit: missing @ here

bmoyles0117 · 2018-08-29T14:18:31Z

lib/fluent/plugin/filter_add_insert_ids.rb

+
+      # Initialize the insertId to a random string.
+      def initialize_insert_id
+        Array.new(INSERT_ID_SIZE) { ALLOWED_CHARS.sample }.join


If the method is named initialize_, it would make more sense to me if the actual assignment occurred here. WDYT?

@insert_id = Array.new(INSERT_ID_SIZE) { ALLOWED_CHARS.sample }.join

I was the one who proposed initialize, and it seems like it was a mistake. Maybe initial_insert_id or generate_initial_insert_id would work better. I like the fact that it's assigned in start.

Renamed to generate_initial_insert_id instead.

igorpeshansky · 2018-08-29T16:15:00Z

lib/fluent/plugin/filter_add_insert_ids.rb

+
+      # Initialize the insertId to a random string.
+      def initialize_insert_id
+        Array.new(INSERT_ID_SIZE) { ALLOWED_CHARS.sample }.join


I was the one who proposed initialize, and it seems like it was a mistake. Maybe initial_insert_id or generate_initial_insert_id would work better. I like the fact that it's assigned in start.

igorpeshansky · 2018-08-29T16:19:50Z

lib/fluent/plugin/filter_add_insert_ids.rb

+        # Only generate and add an insertId field if the record is a hash and
+        # the insert ID field is not already set.
+        if record.is_a?(Hash) && !record.key?(@insert_id_key) \
+            && record[@insert_id_key] != ''


I don't think this does what you intended... You probably meant:

if record.is_a?(Hash) && (!record.key?(@insert_id_key) || record[@insert_id_key] == '')

Alternatively, you could use:

if record.is_a?(Hash) && record[@insert_id_key].to_s.empty?

(I think .empty? is preferred to == '' anyway).

Yeah, unit tests failed for this. I just fixed the logic and committed the unit tests.

igorpeshansky · 2018-08-29T16:21:00Z

test/plugin/test_filter_add_insert_ids.rb

+    Fluent::Test.setup
+  end
+
+  def test_configure_insert_id_key


You're right, let's defer.

qingling128

Feedback addressed. PTAL.

qingling128 · 2018-08-29T16:31:15Z

README.rdoc

@@ -23,22 +23,31 @@ will also install and configure the gem.

 == Configuration

+To embed insertIds into log entries, specify <code>type add_insert_ids</code>


qingling128 · 2018-08-29T16:33:23Z

lib/fluent/plugin/filter_add_insert_ids.rb

+
+      # Initialize the insertId to a random string.
+      def initialize_insert_id
+        Array.new(INSERT_ID_SIZE) { ALLOWED_CHARS.sample }.join


Renamed to generate_initial_insert_id instead.

qingling128 · 2018-08-29T20:28:29Z

lib/fluent/plugin/filter_add_insert_ids.rb

+        # Only generate and add an insertId field if the record is a hash and
+        # the insert ID field is not already set.
+        if record.is_a?(Hash) && !record.key?(@insert_id_key) \
+            && record[@insert_id_key] != ''


Yeah, unit tests failed for this. I just fixed the logic and committed the unit tests.

davidbtucker · 2018-08-29T21:12:25Z

test/plugin/test_filter_add_insert_ids.rb

+  end
+
+  def test_add_insert_ids
+    total_entry_count = 1000


Could this number be smaller (like 5)? Or do we need to test with a high volume?

The number was chose to be this high mainly to test against any potential race condition (there should not be).

igorpeshansky

Minor.

igorpeshansky · 2018-08-29T21:46:55Z

lib/fluent/plugin/filter_add_insert_ids.rb

+      # rubocop:disable Style/UnusedMethodArgument
+      def filter(tag, time, record)
+        # Only generate and add an insertId field if the record is a hash and
+        # the insert ID field is not already set. An empty string is considered


[Optional] not already set (or set to an empty string).

igorpeshansky · 2018-08-29T21:47:12Z

test/plugin/test_filter_add_insert_ids.rb

+                 " Only #{unique_insert_ids.size} found."
+  end
+
+  def test_not_add_insert_ids_if_present


test_insert_ids_not_added_if_present.

igorpeshansky

LGTM

qingling128 added 2 commits August 27, 2018 22:19

Add a filter plugin add_insert_ids.

b1ecc92

Add unit tests.

3a0dcca

qingling128 requested review from bmoyles0117, igorpeshansky and davidbtucker August 28, 2018 02:33

googlebot added the cla: yes label Aug 28, 2018

bmoyles0117 reviewed Aug 28, 2018

View reviewed changes

fix @ in doc

cd84c14

qingling128 commented Aug 28, 2018

View reviewed changes

davidbtucker reviewed Aug 28, 2018

View reviewed changes

igorpeshansky requested changes Aug 29, 2018

View reviewed changes

Address feedback.

c620d97

qingling128 commented Aug 29, 2018

View reviewed changes

bmoyles0117 reviewed Aug 29, 2018

View reviewed changes

igorpeshansky requested changes Aug 29, 2018

View reviewed changes

qingling128 added 2 commits August 29, 2018 12:34

address feedback

4d73329

Fix empty string and add tests.

2681e78

qingling128 commented Aug 29, 2018

View reviewed changes

davidbtucker approved these changes Aug 29, 2018

View reviewed changes

bmoyles0117 approved these changes Aug 29, 2018

View reviewed changes

igorpeshansky requested changes Aug 29, 2018

View reviewed changes

minor

8d38e08

igorpeshansky approved these changes Aug 29, 2018

View reviewed changes

qingling128 merged commit 23cfc5b into master Aug 29, 2018

qingling128 deleted the lingshi-filter branch August 30, 2018 19:04

		@@ -23,22 +23,31 @@ will also install and configure the gem.

		== Configuration

		To embed insertIds into log entries, specify <code>type add_insert_ids</code>

Add a filter plugin add_insert_ids. #246

Add a filter plugin add_insert_ids. #246

Conversation

qingling128 commented Aug 28, 2018

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

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

qingling128 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

davidbtucker Aug 28, 2018 • edited by igorpeshansky Loading

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

davidbtucker Aug 28, 2018 • edited by igorpeshansky Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qingling128 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmoyles0117 Aug 29, 2018 • edited by igorpeshansky Loading

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

davidbtucker Aug 28, 2018 •

edited by igorpeshansky

Loading

davidbtucker Aug 28, 2018 •

edited by igorpeshansky

Loading

bmoyles0117 Aug 29, 2018 •

edited by igorpeshansky

Loading