Add Logz.io support #74

idohalevi · 2018-07-03T11:30:17Z

No description provided.

First

* fixing for travis tests * using foreach for Java7 support

idohalevi · 2018-07-04T09:00:20Z

Is it a must to support Java7 before merging?

jakub-bochenski · 2018-07-04T10:54:37Z

I'll do my best to merge #70 ASAP, in the meantime you can just rebase your branch over that one

src/main/java/jenkins/plugins/logstash/persistence/LogstashIndexerDao.java

src/main/java/jenkins/plugins/logstash/LogstashConfiguration.java

src/main/java/jenkins/plugins/logstash/configuration/Logzio.java

src/main/java/jenkins/plugins/logstash/persistence/LogzioDao.java

src/test/java/jenkins/plugins/logstash/LogstashConfigurationMigrationTest.java

src/main/java/jenkins/plugins/logstash/persistence/LogzioDao.java

src/test/java/jenkins/plugins/logstash/persistence/LogzioDaoTest.java

idohalevi · 2018-09-12T09:01:18Z

@mwinter69 @jakub-bochenski Done. I did find a problem with Collections.synchronizedList when I tested with 2 parallel builds I got an exception java.util.ConcurrentModificationException. The cause for it is the iterator in the function sizeInBytes from the logzio sender. If it's ok I would like to use CopyOnWriteArrayList instead as it looks like it fixes the issue

jakub-bochenski · 2018-09-13T16:14:38Z

@idohalevi I don't think synchronizing on the list is enough
You can still get problems e.g if one thread calls send() while the other is in the middle of clear() we can get lost messages.

I see two approaches here:

make all mutating methods synchronized
use a new instance for each build

One more problem I see with reusing the instance is: if an un-sendable message (one that is causing an exception in sendToLogzio) gets into the list it will be stuck there permanently blocking all builds from sending data.

idohalevi · 2018-09-13T19:56:39Z

@jakub-bochenski @mwinter69
I will work on making a shipper per build. One question, how will you suggest I should handle closing shippers? is there a way I can know that the build is over? maybe I can rely on the success/fail results?

jakub-bochenski · 2018-09-14T12:29:15Z

@idohalevi follow the changes here: https://github.com/jenkinsci/logstash-plugin/pull/71/files#diff-bddec744c0206f7f355a27d666bfda1f
in #71 the author has added a close() method to be able to finalize processing

…into dev # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit. update from upstream

Dev

…obs) problems

add close method

idohalevi · 2019-03-18T14:59:17Z

@jakub-bochenski Done:) --- sorry for taking the time to handle this...

jakub-bochenski · 2019-03-18T15:03:50Z

No worries, thanks for taking the time to finish it

pom.xml

src/main/java/jenkins/plugins/logstash/LogstashBuildWrapper.java

jakub-bochenski · 2019-03-18T15:47:16Z

This shouldn't include the whole of #71, please just add the part required for the close() method

idohalevi · 2019-03-18T15:50:25Z

@jakub-bochenski
Thank you for the quick reply.
Will work on it and get back to you

remove unused methods

pom.xml

jakub-bochenski · 2019-03-19T12:52:45Z

src/main/java/jenkins/plugins/logstash/LogstashScriptProcessor.java

+ * return any cached but non-sent data back for persisting.
+ * The return value of script is the payload to be persisted unless null.
+ */
+public class LogstashScriptProcessor implements LogstashPayloadProcessor{


I don't think you need this implmentation, just the interface

Even the interface is not needed. Actually the complete #71 should be taken out of here.

We need a close hook for the build. It's not available on master, so I suggested to use the same approach as #71

What helps an interface when there is no implementation?
The close is added to the LogstashWriter in order to properly terminate the LogstashPayloadProcessor.
#71 modifies the ConsoleLogFilter to add the PayloadProcessor to the LogstashWriter.
There is nothing comparable here.

How do you suggest we should continue with this?

Let me try to explain again.
This code needs to perform actions on build close as per #74 (comment)
AFAIK we have currently no way to notify an IndexerDao that a build has finished.
#71 added a way to do it, so I suggested to reuse the same approach.

Maybe at this point the best way to proceed is to forget about #71 and just add a way to notify the IndexerDao.
We can worry about the overlap with #71 when/if it's merged

#71 added a feature to close the LogstashWriter, which is specific to a build but it is not interacting with the Dao as the scriptprocessor is only used in the writer.
The dao is a kind of a singleton in Jenkins. We only create a new Dao, when we have a configuration change and the new will only be used for new builds.
If I understand it right the shipper has to be used in the dao. So when the writer calls dao.push which does the actual writing to the indexer, how does the dao know which shipper to use? I don't see any nice solution here other than having a dao per build. But this would be a change in concept how this plugin is working. The writer would hold the reference to the dao and can be easily closed.
This is a bigger change in how the plugin works.
But even then one will need synchronization as in pipeline jobs you also have parallel execution within a single build and thus concurrent calls to the list.

Sorry, I think I meant the LogstashWriter instead of IndexerDao
The idea is to create a new shipper instance per build to avoid concurrency issues. Not sure if that would work for parallel stages in pipeline though.

On a different note: I'm rather busy lately and don't want to block it. I would be happy to accept whatever solution you and @idohalevi can agree on

@jakub-bochenski @mwinter69
How do you suggest to continue?

jakub-bochenski · 2019-03-19T12:53:29Z

src/main/java/jenkins/plugins/logstash/LogstashPayloadProcessor.java

+     * @param payload the JSON payload that has been constructed so far.
+     * @return The formatted JSON object, can be null to ignore this payload.
+     */
+    JSONObject process(JSONObject payload) throws Exception;


I think we actually only need the finish() method, not process().

@jakub-bochenski
Currently, finish() uses process(). How would you like it to be instead?

AFAIR finish only uses process to call it with null arg. Anyhow check the comment above (#74 (comment)) it might make this point moot

jakub-bochenski · 2019-03-19T12:54:58Z

I think it's still pulling in too much. Maybe an easier approach would be to take your original version and try to pick the needed changes onto it from scratch instead of trying to modify current state.
Let me know if you need help here

mwinter69 · 2019-03-19T20:22:48Z

src/main/java/jenkins/plugins/logstash/LogstashBuildWrapper.java

    }
-  }


Please revert the complete file to its original state. You just changed indentation.

idohalevi and others added 21 commits May 31, 2018 14:24

first

d986d80

queue in workspace

8a00ac8

deleting comments

de18b60

PR fixes

4ef9a2f

change dir path

8c185d6

using tmp dir

6c7d6a6

change @timestamp field name

9b7621d

using stream

5075433

Merge pull request #1 from idohalevi/first

fa317c1

First

Update Logzio.java

bf87eec

add debug info

6d53f29

change queue location

4d9134d

change const

54cd262

add debug

7a9164d

debug prints

959f715

debug

03cb931

gzip logs

035fb68

Merge branch 'master' of https://github.com/jenkinsci/logstash-plugin

53ffac5

delete debug print

84aed45

fixing for travis tests (#2)

193f057

Fix travis (#3)

4dbe73e

* fixing for travis tests * using foreach for Java7 support

mwinter69 reviewed Jul 5, 2018

View reviewed changes