Closed contentfree closed 8 years ago
Hmm... I'd signed the CLA. Looking at the commit log above, I think that it might be another contributor to my fork that hasn't signed. Will rebase…
I still need to run the tests to verify that they still work
Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.
Impatiently awaiting this fix! :)
It looks like this just needs a review, is this likely to be approved soon?
oh pls pls i need dis! thanks!
+1
+1
However I have a question how to configure this plugin to handle data that already comes in InfluxDB line format?
e.g.
system_load1,host=debian value=0 1449261770042724367
system_load5,host=debian value=0.01 1449261770042741446
system_load15,host=debian value=0.05 1449261770042747442
I been testing out this branch, I think need to modify this a little bit to handle strings better or needs to stick them in double quotes so it can handle spaces in the string. https://influxdb.com/docs/v0.9/write_protocols/line.html
@timgriffiths To handle a field called task_name that contains spaces I added the following filter to my conf file. This adds an escape character where a space occurs.
mutate { gsub => ["task_name"," ","\ "]}
Here is the logstash reference to mutate. https://www.elastic.co/guide/en/logstash/current/plugins-filters-mutate.html#plugins-filters-mutate-gsub
Yep that's how i have worked around this for now, but i think the plugin should just handle this it's part of the protocol
jenkins, test this
Code overall LGTM. I left a few inline comments about backwards compatiibily and versioning.
Looks like the test suite failed - check out the failure under 'Failures:' - one test is failing:
es:
1) LogStash::Outputs::InfluxDB complete pipeline run with 2 events should receive 2 events, flush and call post with 2 items json array
Failure/Error: subject.close
LogStash::Outputs::InfluxDB: {"host"=>"localhost", "user"=>"someuser", "password"=><password>, "allow_time_override"=>true, "data_points"=>{"foo"=>"%{foo}", "bar"=>"%{bar}", "time"=>"%{time}"}, "codec"=><LogStash::Codecs::Plain charset=>"UTF-8">, "workers"=>1, "db"=>"statistics", "retention_policy"=>"default", "port"=>8086, "measurement"=>"logstash", "time_precision"=>"ms", "coerce_values"=>{}, "use_event_fields_for_data_points"=>false, "exclude_fields"=>["@timestamp", "@version", "sequence", "message", "type"], "send_as_tags"=>["host"], "flush_size"=>100, "idle_flush_time"=>1} received :post with unexpected arguments
expected: ("[{\"name\":\"logstash\",\"columns\":[\"foo\",\"bar\",\"time\"],\"points\":[[\"1\",\"2\",\"3\"],[\"1\",\"2\",\"3\"]]}]")
got: ("logstash foo=\"1\",bar=\"2\" 3\nlogstash foo=\"1\",bar=\"2\" 3")
Please stub a default value first if message might be received with other args as well.
# ./lib/logstash/outputs/influxdb.rb:171:in `flush'
# ./lib/logstash/outputs/influxdb.rb:212:in `close'
# ./spec/outputs/influxdb_spec.rb:36:in `(root)'
Finished in 0.827 seconds (files took 2.37 seconds to load)
5 examples, 1 failure
@jordansissel The test failure's expected values: ("[{\"name\":\"logstash\",\"columns\":[\"foo\",\"bar\",\"time\"],\"points\":[[\"1\",\"2\",\"3\"],[\"1\",\"2\",\"3\"]]}]") <---- This is looks like the old influxdb 0.8.x write protocol
For influxdb 0.9.x the write line protocol has changed what you got got: ("logstash foo=\"1\",bar=\"2\" 3\nlogstash foo=\"1\",bar=\"2\" 3") Here is the documentation links https://docs.influxdata.com/influxdb/v0.9/write_protocols/write_syntax/ https://docs.influxdata.com/influxdb/v0.9/write_protocols/json/
Hope that helps
@contentfree the first until test is broken as it's still expecting the result to be json but you have converted it all across to the new line protocol
@jordansissel what's status of this PR?
👍🏻
merge! merge! merge!
@timgriffiths can you sign the CLA since you have a commit for the broken unit tests? This is step2 of https://github.com/elasticsearch/logstash/blob/master/CONTRIBUTING.md#contribution-steps.
@contentfree FYI ^^
We will not be able to merge until the CLA has been signed, or the commit fc3549d has been replaced by someone who has signed the CLA
@suyograo So i have signed the CLA, I have also created #30 which covers all the good work @contentfree present in this pull, but I have fixed the unit test and handle spaces and comma's and equal signs according to the influx line protocol all unit tested out as well.
+1
Hope this gets merged soon. Using tag keys would be very useful.
+1
:100:
Guys, someone must do something with this PR.
influxdb 0.10 was released a few days ago...
@contentfree and @timgriffiths finally merged https://github.com/logstash-plugins/logstash-output-influxdb/pull/30. Thanks for your patience.
@contentfree and @timgriffiths would you be interested in helping maintain this plugin?
@contentfree thanks! Can you please a) rebase? b) sign the CLA https://github.com/elasticsearch/logstash/blob/master/CONTRIBUTING.md#contribution-steps