allow scripted changes to logged data #71

akostadinov · 2018-06-18T20:13:52Z

continuation of #56

I resolved conflicts but I didn't test and for some reason test suite hangs
with 100% CPU on my machine (LogstashIntegrationTest.java). I'm wondering whether CI will be better.

In any case need to test before merging even if CI succeeds.

~~update: tests seem to pass in github CI but some other strange JVM error, I'll maybe have to remove the added deps here.~~
update: Presently things should be fixed. I see all tests pass locally. Need to check everything is correctly migrated from my original PR. Then I need to implement some requested changes from it.
update: Removed suppressions because they are fixed in the codebase already; remains to go over relevant comments in #40 to implement the necessary changes.

mwinter69

We also have now the option to enable globally. Maybe it should also be able to define the script globally with the option to disable or overwrite with project specific script.
And what about the LogstashNotifier and the pipeline step. Shouldn't it be possible to add the script there as well?

mwinter69 · 2018-06-22T05:57:42Z

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

@@ -48,13 +58,21 @@
 public class LogstashBuildWrapper extends BuildWrapper
 {

+  @CheckForNull


LogstashBuildWrapper is the wrong place. This class is deprecated and only here for backwards compatibility.
You have to put this in the LogstashJobProperty.

hmm, thanks, I'll investigate what is this LogstashJobProperty. Please check out other comments as I'm more concerned about decisions around script handling.

LogstashJobProperty is the place were the script needs to be defined.
LogstashConsoleLogFilter is the place were the LogstashWriter is created when enabled globally, set via JobProperty or for the pipeline step.

akostadinov · 2018-06-22T07:40:29Z

@mwinter69 , I'm not sure a global script would make sense. Is global option to enable the wrapper or the post build action? wrt pipeline step - I think it should be possible to use script there.

My desire is to

merge wrapper support
implement the post-build action support
implement perf improvements via script-security plugin update

If people think a global script is needed, then that shouldn't be hard to do I guess. Just figuring out some details around how script should work are distracting. That's why would like to see initial feature merged, then implement in the other places.

I hope to finish current feature today and see if anyting needs to change. Hope to update this PR in an hour or two.

Update: couldn't make printing to console nicer. Binding a Closure as echo and then calling echo "my script" results in

RejectedAccessException: Scripts not permitted to use method groovy.lang.GroovyObject invokeMethod java.lang.String java.lang.Object (Script1 echo java.lang.String)

I couldn't get around it.

Please see line comments for things to pay attention to.

akostadinov · 2018-06-22T10:45:52Z

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

+    String scriptString =
+      "if (payload['message']) {\n" +
+      "  if (payload['message'][0] =~ /" + ignoredMsg + "/) {\n" +
+      "    payload.clear()\n" +


To avoid assigning null I thought calling clear() makes sense. It would serve as a Null object. Do you like this or still want payload = SKIP?

payload.clear() is fine

akostadinov · 2018-06-22T10:55:16Z

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

+    String scriptString =
+      "if (payload['message']) {\n" +
+      "  if (payload['message'][0] =~ /" + ignoredMsg + "/) {\n" +
+      "    payload.clear()\n" +


Another thing here is that in #40 it was requested to get returned value instead of setting payload variable. The premises were that users will usually deal with the messages. But later on it turned out users are more interested in modifying metadata. This now can be done with:

payload["my-new-field"] = "value1" payload.discard("undesired-field")

And this would be the whole script. With return values one needs to be careful to add proper return statement. Especially when there are if/else conditions.

Additionally, if we accept return values, when doing if/else it is easy to forget and return null by the script. Then how should we handle the null? As to skip persisting or raise an error? If you tell me you want the return approach, I'll do, just tell me how to handle null returns.

Another implication is that if we do not use the return approach then accepting regular expression patterns or matchers will not be sane after that. But if users are mainly interested in metadata is this a problem?

Finally, we want this script functionality to be used in the post-build action, then how would a matcher work there? Does it make sense to apply matcher to each message in the message array?

I'm really in doubt what is better. I think I prefer a little bit the current approach as it makes harder to get it wrong IMO. But there are some upsides of the return approach as well. So sorry for asking so many times. If you still prefer return I'll go for the return.

OK, lets do it they way you are proposing
I guess if we want concise message matcher we can expose it via a separate attribute etc.

Thank you! will be going for a couple of days in the wild but plan to make this migrated to LogstashJobProperty next week.

yeah, unfortunately I'm going to the wild for the next 2 weeks on Sat

akostadinov · 2018-06-22T10:55:53Z

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

+      "  }\n" +
+      "  lastPayload = payload\n" +
+      "} else {\n" +
+      "  console.println('test build console message')\n" +


no luck with binding a Closure see #40 (comment)

akostadinov · 2018-06-22T10:57:12Z

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

+  private Boolean payloadNeedsPersistence(JSONObject payload) {
+    if (payload != null) {
+      Object message = payload.opt("message");
+      return message != null && (message instanceof JSONArray) && !((JSONArray) message).isEmpty();


Basically if there are messages in whatever script returns, only then we persist. Without messages we ignore the payload.

akostadinov · 2018-06-22T11:00:55Z

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

+   */
+  @Override
+  public JSONObject process(JSONObject payload) throws Exception {
+    binding.setVariable("payload", payload);


In #40 we spoke about possibly duplicating the payload so original one is intact. For small payload objects this might be acceptable but if we apply same to post-build action, then the object might be quiet large. Also dup operation may not be so quick. And I don't see some immediate benefit of doing this additional processing. So leaving as is unless there are objections.

jakub-bochenski · 2018-08-08T14:02:47Z

@akostadinov do you need help here or just more time?

akostadinov · 2018-08-08T20:56:35Z

@jakub-bochenski , I spent some time trying to figure out how to enable the function as a JobProperty. And this is my uneducated attempt at it. I see the configuration in job config and I can enter a script but when I save nothing happens. I mean script is not saved and when I open job config again, the script field is still empty.

The second issue is that after 75d7aa8, the test suite hangs at jenkins.plugins.logstash.LogstashIntegrationTest for some unknown yet reason.

Finally my changes to the JobStep are totally wild. I have no idea whether I'm on the right track at all.

So if you can give any advise about any of those, it will greatly help.

mwinter69 · 2018-08-09T12:17:03Z

src/main/resources/jenkins/plugins/logstash/LogstashJobProperty/config.jelly

@@ -1,5 +1,9 @@
 <?jelly escape-by-default='true'?>
 <j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form">
 	<f:optionalBlock name="enable" title="${descriptor.displayName}" checked="${instance != null}" help="/plugin/logstash/help/help.html">
+		<f:advanced>
+			<f:entry field="script" help="/plugin/logstash/help/help-script.html"/>
+			<f:property field="secureGroovyScript"/>


field name doesn't match the variable name, this causes your problems with saving
secureGroovyScript <=> securedGroovyScipt

@mwhudson , sorry but I don't see securedGroovyScipt used in the code anywhere. Also when field doesn't match class an exception is thrown. Or did I miss your point?

you're right, must have had tomatoes on my eyes
Can you try to add an inline=true to the optionalBlock. Without it the nested stuff is not at the right place in the generated json

<f:optionalBlock name="enable" title="${descriptor.displayName}" checked="${instance != null}" help="/plugin/logstash/help/help.html" inline="true">

Still didn't work :/

akostadinov · 2018-09-24T20:16:07Z

Interesting, I switched to OptionalJobProperty and things started to work. I mean configuration is properly saved at least. I'm yet to check whether script is working and the other issues from my previous comment.

jakub-bochenski · 2018-10-08T11:38:45Z

@akostadinov I'm glad to hear you are making progress and I'm sorry I wasn't able to offer more assistance lately.

I see the current build is failing because of findbugs, feel free to ignore the issues for now, we can iron them out later

denglitong · 2019-01-15T03:16:18Z

so, how is it going now?

jakub-bochenski · 2019-01-18T16:00:38Z

@denglitong sadly I haven't heard from @akostadinov for a while now.

Personally I'm spending all my available OSS time on https://github.com/jenkinsci/stash-pullrequest-builder-plugin as it's an everyday pain at my work.
If things go right I expect I will contribute some more work where later this quarter.

As this is open source you are of course invited to lend a hand. If you need help getting started let me know

akostadinov mentioned this pull request Jun 18, 2018

support custom log lines processing via script #56

Closed

akostadinov force-pushed the master branch 2 times, most recently from d3fa327 to 20f4a6a Compare June 21, 2018 13:09

mwinter69 suggested changes Jun 22, 2018

View reviewed changes

akostadinov commented Jun 22, 2018

View reviewed changes

jakub-bochenski mentioned this pull request Jul 6, 2018

Add Logz.io support #74

Open

akostadinov added 4 commits July 31, 2018 23:24

allow scripted changes to logged data

db9f94e

remove suppressions

3f54df3

avoid passing null and better classloader

321ff7b

first wild code dump to get script supported in JobProperty

a34d055

jakub-bochenski mentioned this pull request Aug 8, 2018

Add passing tests and test duration #80

Closed

second wild dump

75d7aa8

akostadinov force-pushed the master branch from 2980228 to 75d7aa8 Compare August 8, 2018 20:50

mwinter69 reviewed Aug 9, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow scripted changes to logged data #71

allow scripted changes to logged data #71

akostadinov commented Jun 18, 2018 •

edited

Loading

mwinter69 left a comment

mwinter69 Jun 22, 2018

akostadinov Jun 22, 2018

mwinter69 Jun 24, 2018

akostadinov commented Jun 22, 2018 •

edited

Loading

akostadinov Jun 22, 2018

jakub-bochenski Jul 11, 2018

akostadinov Jun 22, 2018

jakub-bochenski Jul 11, 2018

akostadinov Jul 12, 2018

jakub-bochenski Jul 12, 2018

akostadinov Jun 22, 2018

akostadinov Jun 22, 2018

akostadinov Jun 22, 2018

jakub-bochenski commented Aug 8, 2018

akostadinov commented Aug 8, 2018

mwinter69 Aug 9, 2018

akostadinov Aug 9, 2018

mwinter69 Aug 9, 2018

akostadinov Aug 23, 2018

akostadinov commented Sep 24, 2018

jakub-bochenski commented Oct 8, 2018

denglitong commented Jan 15, 2019

jakub-bochenski commented Jan 18, 2019

allow scripted changes to logged data #71

Are you sure you want to change the base?

allow scripted changes to logged data #71

Conversation

akostadinov commented Jun 18, 2018 • edited Loading

mwinter69 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

akostadinov commented Jun 22, 2018 • edited 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

jakub-bochenski commented Aug 8, 2018

akostadinov commented Aug 8, 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

akostadinov commented Sep 24, 2018

jakub-bochenski commented Oct 8, 2018

denglitong commented Jan 15, 2019

jakub-bochenski commented Jan 18, 2019

akostadinov commented Jun 18, 2018 •

edited

Loading

akostadinov commented Jun 22, 2018 •

edited

Loading