Support for multiple tags #31

chergey · 2018-06-09T12:10:39Z

I added support for multiple tags.

…-output-loggly # Conflicts: # logstash-output-loggly.gemspec

webmat · 2018-06-12T15:23:49Z

Thanks for taking the time to work on this.

Currently it's already possible to specify multiple tags, as mentioned in the doc.

What's the issue you're trying to fix? Interpolation like the following should work as well, even with multiple tags:

output {
  loggly { ... "tag" => "tag1,%{log_type}" }
}

Or have you encountered issues where this didn't work as expected?

chergey · 2018-06-12T17:21:34Z

output {
  loggly { ... "tag" => "tag1,%{log_type}" }
 }

That didn't work for me (defaut tag ("logstash") was overriding).

webmat · 2018-06-14T17:59:15Z

The problem likely comes from your field log_type not being present.

If I start Logstash with the following:

bin/logstash -e "input { stdin {} generator { count => 1 } }
  output {  loggly { key => \"$LOGGLY_KEY\" tag => 'logstash,tag_%{sequence}' } }

The generator plugin will generate one event with field "sequence":0. The event correctly appears in Loggly with tags logstash and tag_0.

However if I type another message, the stdin input doesn't set a sequence field. Since the failed interpolation leaves the tag_%{sequence} in place, and this bit of code comes down like a hammer, and replaces the whole tag attribute with the default tag. This is indeed not ideal.

webmat · 2018-06-14T18:19:40Z

However I would like it if you could do a few things in your PR, prior to merging this.

Make sure to de-duplicate the tag list by calling .uniq on it, in case the list of tags ends up being "logstash,logstash,mylog", in case of interpolation problems.
Please add tests for the various scenarios covered by this improved support for multiple tags. Here are some ideas:
- Unit tests around scenarios of calling prepare_meta for different scenarios: no interpolation, interpolation of one tag, interpolation of one tag in a list of tags, a failed interpolation, duplicate tags are deduped.
  - Here is an example of unit tests directly calling prepare_meta, it may give you some ideas on how to get started.
- Tests around the grouping of API calls based on different mixes of tags. Since the API requires the tag list to be part of the URI, and since the plugin accepts batches of events that can all have different lists of tags, we have to group the calls to do one bulk API call per pair of key + tag list.
  - Here are the current tests of splitting the calls in the proper key+tag list buckets. You can either add to those or create a new section.

If you're not familiar with RSpec, our testing framework, you can always check this site, the Expectations and Mocks sections will likely help you figure out how to start from our existing tests and add new ones.

chergey · 2018-06-16T01:24:33Z

@webmat, thanks, I'll look into your your example.

webmat · 2018-06-19T15:56:52Z

@chergey Also, could you make sure you sign our Contributor License Agreement, please? https://www.elastic.co/contributor-agreement

chergey · 2018-06-20T03:00:47Z

@webmat, how about we remove default tag ('logstash') ? If no tags supplied or failed interpolation, then no tag will result, e.g. %{tag1},tag2 would be 'tag2' (field 'tag1' doesn't exist)

webmat · 2018-06-20T03:12:05Z

Yes that makes sense.

Make sure to still default to 'logstash' if ever the final tag is an empty string (one or more interpolations failed + no literal tag). An example to make sure I'm clear:

#  fail    fail
'%{tag1},%{tag2}'

Should end up with tag logstash :-)

webmat · 2018-06-20T03:15:12Z

In other words, inside the loop, just don't add the tag if it didn't interpolate correctly (e.g. if it still matches the regex).

And only after the loop do you check whether it's necessary to default to 'logstash' (if array is empty).

chergey · 2018-06-20T03:40:30Z

@webmat, why still default to 'logstash' (if it didn't interpolate correctly) ? We can send events without any tag at all i.e. http://logs-01.loggly.com/inputs/TOKEN.

webmat · 2018-06-20T13:58:24Z

Ah I was not aware that the tag was optional. Their documentation doesn't clearly state whether the tag is required or can be omitted.

If that is so, I don't mind sending with no tag. Make sure that the URL will be adjusted accordingly (I would assume that "...TOKEN/tag/" with no tags won't work.

And document the new behaviour in the comments for the attribute and in docs/index.asciidoc. GitHub renders asciidoc just like markdown, so you can check that what you did is fine (even if GitHub doesn't render the includes & so on at the top of the file).

chergey · 2018-06-22T05:42:43Z

@webmat, please comment on my commit.

webmat

Sorry for the delay. Hectic past few days over here :-)

Good work overall. I have a few requests, but once these are taken care of, I think we'll be ready to merge and release :-)

You did specifically confirm that it's fine to send API calls without the tag/#{tag}, correct?

webmat · 2018-06-27T18:47:48Z

lib/logstash/outputs/loggly.rb

+      # For those cases where %{somefield} doesn't exist we don't include it
+      if !/%{\w+}/.match(t)
+        tag_array.push(t)
+      end


This will not add tags that failed interpolation, this is good.

However this would still add empty string tags to the array, in cases where an interpolated field contains an empty string.

Also minor code convention nitpick. Avoid if ! and use unless instead.

webmat · 2018-06-27T18:52:35Z

lib/logstash/outputs/loggly.rb


    if expected_field = key[/%{(.*)}/, 1]
      @logger.warn "Skipping sending message to Loggly. No key provided (key='#{key}'). Make sure to set field '#{expected_field}'."
      @logger.debug "Dropped message", :event => event.to_json
      return nil
    end

-    tag=tag_array.join(",")
+    tag = nil


Declaring the variable outside the if is not necessary outside of control blocks like if. This is only necessary for blocks like do...end.

webmat · 2018-06-27T18:55:18Z

spec/outputs/loggly_spec.rb

-    it 'should set the default tag to logstash' do
-      expect(output).to receive(:send_batch).with([{event: event.to_hash, key: 'abcdef123456', tag: 'logstash'}])
-      output.receive(event)
-    end


This test for the default behaviour should not be removed. Rather it should be adapted to show the new default behaviour (no tag and the URI ending in /bulk/#{key} without the /tag/#{tag}).

webmat · 2018-06-27T19:03:45Z

lib/logstash/outputs/loggly.rb

+        url = "#{@proto}://#{@host}/bulk/#{key}/tag/#{tag}"
+      else
+        url = "#{@proto}://#{@host}/bulk/#{key}"
+      end


Once again, avoid if !. unless...else is not better, though. So perhaps use something like this instead?

if tag.nil? url = "#{@proto}://#{@host}/bulk/#{key}" else url = "#{@proto}://#{@host}/bulk/#{key}/tag/#{tag}" end

webmat · 2018-06-27T19:06:44Z

spec/outputs/loggly_spec.rb

@@ -60,9 +56,9 @@ def logger_for(plugin)
      output.receive(event)
    end

-    it 'should default tag to logstash if interpolated field for tag does not exist' do
+    it 'should send no tag to logstash if interpolated field for tag does not exist' do


Initial wording (before your contribution) was a bit wonky. Could you remove the inaccurate "to logstash", and perhaps have this instead?

it 'should have no tag if the interpolated field for the tag does not exist' do

webmat · 2018-06-27T19:18:23Z

spec/outputs/loggly_spec.rb

@@ -110,6 +106,46 @@ def logger_for(plugin)
      end
    end

+    context 'prepare tags' do
+      it 'failed interpolation' do


Your context and it strings should read more like sentences that describe the situation you're setting up (context) and intended result (it).

Here's an example for the two lines above:

context 'when computing the tags' it 'should not leave a failed interpolation in the tag list' do

then of course all following it blocks should be clarified to describe expected behaviour of each test case as well.

webmat · 2018-06-27T19:38:49Z

spec/outputs/loggly_spec.rb

+        meta_event = output.send :prepare_meta, event
+        expect(meta_event[:tag]).to eq('some_value')
+      end
+    end


Good coverage of the various scenarios, however. Love it 💯

chergey · 2018-06-27T23:06:28Z

You did specifically confirm that it's fine to send API calls without the tag/#{tag}, correct?

Yes, we have our api in java that works just fine. Besides, I re-checked it myself with rest client.

Declaring the variable outside the if is not necessary outside of control blocks like if. This is only necessary for blocks like do...end

You sure, it's ok ? Intellij IDEA Ruby plugin complains.

webmat · 2018-06-28T13:43:35Z

Excellent. Once again, thanks a lot for your contribution!

I will merge and release the next version of the plugin.

webmat · 2018-06-28T13:49:44Z

Ah I have a small bit of housekeeping for you first, though.

I see you've signed our CLA with your email @mail.ru, but not with your email for the first commits @haulmon.com. Could you sign the CLA with the latter email as well, please? http://www.elasticsearch.org/contributor-agreement/

chergey · 2018-06-28T18:58:36Z

I see you've signed our CLA with your email @mail.ru, but not with your email for the first commits @haulmon.com. Could you sign the CLA with the latter email as well, please? http://www.elasticsearch.org/contributor-agreement/

Done.

webmat · 2018-06-28T19:16:14Z

Thanks, I will be releasing this shortly!

webmat · 2018-07-03T17:54:46Z

@chergey Version 6.0.0 with your changes is finally out (after a few internal back & forths :-) ). Once again, thanks a lot for taking the time to contribute!

chergey added 4 commits June 9, 2018 15:55

added support for multiple tags

5905bb3

Merge branch 'master' of https://github.com/logstash-plugins/logstash…

a4b5079

…-output-loggly # Conflicts: # logstash-output-loggly.gemspec

add support for multiple tags

76ceeeb

add support for multiple tags

8f31ac1

jsvd requested a review from webmat June 12, 2018 14:52

webmat assigned webmat and chergey Jun 19, 2018

better support for tag interpolation

2f8a969

webmat suggested changes Jun 27, 2018

View reviewed changes

better support for tag interpolation

328add4

webmat approved these changes Jun 28, 2018

View reviewed changes

webmat merged commit 22a20da into logstash-plugins:master Jun 28, 2018

webmat mentioned this pull request Jun 28, 2018

Release version with improved tag support #32

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for multiple tags #31

Support for multiple tags #31

chergey commented Jun 9, 2018

webmat commented Jun 12, 2018

chergey commented Jun 12, 2018 •

edited

Loading

webmat commented Jun 14, 2018

webmat commented Jun 14, 2018 •

edited

Loading

chergey commented Jun 16, 2018

webmat commented Jun 19, 2018

chergey commented Jun 20, 2018 •

edited

Loading

webmat commented Jun 20, 2018

webmat commented Jun 20, 2018

chergey commented Jun 20, 2018 •

edited

Loading

webmat commented Jun 20, 2018

chergey commented Jun 22, 2018 •

edited

Loading

webmat left a comment

webmat Jun 27, 2018 •

edited

Loading

webmat Jun 27, 2018

webmat Jun 27, 2018

webmat Jun 27, 2018

webmat Jun 27, 2018

webmat Jun 27, 2018

webmat Jun 27, 2018

webmat Jun 27, 2018

chergey commented Jun 27, 2018 •

edited

Loading

webmat commented Jun 28, 2018

webmat commented Jun 28, 2018

chergey commented Jun 28, 2018 •

edited

Loading

webmat commented Jun 28, 2018

webmat commented Jul 3, 2018

Support for multiple tags #31

Support for multiple tags #31

Conversation

chergey commented Jun 9, 2018

webmat commented Jun 12, 2018

chergey commented Jun 12, 2018 • edited Loading

webmat commented Jun 14, 2018

webmat commented Jun 14, 2018 • edited Loading

chergey commented Jun 16, 2018

webmat commented Jun 19, 2018

chergey commented Jun 20, 2018 • edited Loading

webmat commented Jun 20, 2018

webmat commented Jun 20, 2018

chergey commented Jun 20, 2018 • edited Loading

webmat commented Jun 20, 2018

chergey commented Jun 22, 2018 • edited Loading

webmat left a comment

Choose a reason for hiding this comment

webmat Jun 27, 2018 • edited Loading

Choose a reason for hiding this comment

webmat Jun 27, 2018

Choose a reason for hiding this comment

webmat Jun 27, 2018

Choose a reason for hiding this comment

webmat Jun 27, 2018

Choose a reason for hiding this comment

webmat Jun 27, 2018

Choose a reason for hiding this comment

webmat Jun 27, 2018

Choose a reason for hiding this comment

webmat Jun 27, 2018

Choose a reason for hiding this comment

webmat Jun 27, 2018

Choose a reason for hiding this comment

chergey commented Jun 27, 2018 • edited Loading

webmat commented Jun 28, 2018

webmat commented Jun 28, 2018

chergey commented Jun 28, 2018 • edited Loading

webmat commented Jun 28, 2018

webmat commented Jul 3, 2018

chergey commented Jun 12, 2018 •

edited

Loading

webmat commented Jun 14, 2018 •

edited

Loading

chergey commented Jun 20, 2018 •

edited

Loading

chergey commented Jun 20, 2018 •

edited

Loading

chergey commented Jun 22, 2018 •

edited

Loading

webmat Jun 27, 2018 •

edited

Loading

chergey commented Jun 27, 2018 •

edited

Loading

chergey commented Jun 28, 2018 •

edited

Loading