support custom log lines processing via script #40

akostadinov · 2017-12-12T23:05:26Z

This change requires jenkinsci/script-security-plugin#169 to work.

Please see help-script.html for possible use cases.

I needed to update parent pom version because of script-security-plugin which led to
the need to suppress additional warnings. Still waiting on
jenkinsci/script-security-plugin#169 to be accepted so not ready to merge. I have an
alternative approach if the pull is not accepted, unfortunately not that performant.

But I will appreciate feedback for this PR in the meanwhile so I can fix issues.

Thank you.

oleg-nenashev

Binary compatibility in the constructor needs to be fixed. Some FindBugs issues could be also cleaned up

oleg-nenashev · 2017-12-12T23:16:16Z

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

  /**
   * Create a new {@link LogstashBuildWrapper}.
   */
  @DataBoundConstructor
-  public LogstashBuildWrapper() {}
+  public LogstashBuildWrapper(
+      @CheckForNull SecureGroovyScript secureGroovyScript


Breaks the binary compatibility. The default constructor should be retained

I'm very new to plugin dev task. My gut feeling told me that given jelly is changed, the default constructor will never be called by Jenkins thus retaining a parameterless constructor would be useless.
If you want me to keep one I'm fine with it. Just tell me whether it should be annotated as @DataBoundConstructor or not. Other than that, it would be an easy change.

oleg-nenashev · 2017-12-12T23:16:40Z

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

@@ -106,9 +118,18 @@ public DescriptorImpl getDescriptor() {
    return (DescriptorImpl) super.getDescriptor();
  }

+  public SecureGroovyScript getSecureGroovyScript() {


@CheckForNull would be useful

I'll look at this. Thanks.

oleg-nenashev · 2017-12-12T23:17:56Z

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

@@ -65,6 +67,9 @@ public static Descriptor getLogstashDescriptor() {
  public static final class Descriptor extends ToolDescriptor<LogstashInstallation> {
    public IndexerType type;
    public SyslogFormat syslogFormat;
+    @SuppressFBWarnings(
+      value="UUF_UNUSED_PUBLIC_OR_PROTECTED_FIELD",
+      justification="TODO: do we need this?")


If you decide to delete it, make it transient first.

I would prefer if this findbug issues are fixed in another pull request as this one is large enough.
I will look at the date formatter issues while waiting for script security plugin though as they appear to be more critical.

And thank you for the quick turnaround!

oleg-nenashev · 2017-12-12T23:22:01Z

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

@@ -57,6 +59,9 @@
  }

  @Override
+  @SuppressFBWarnings(
+    value="STCAL_INVOKE_ON_STATIC_DATE_FORMAT_INSTANCE",
+    justification="TODO: not sure how to fix this")


This is a real defect. SimpleDateFormar is not thread-safe, so this code may fall apart at any moment.
I would recommend just instantiating the object instead of using the static field

oleg-nenashev · 2017-12-12T23:22:30Z

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

@@ -65,6 +67,9 @@
 */
 public class BuildData {
  // ISO 8601 date format
+  @SuppressFBWarnings(
+    value="STCAL_STATIC_SIMPLE_DATE_FORMAT_INSTANCE",
+    justification="TODO: not sure how to fix this")


akostadinov · 2017-12-13T13:26:12Z

Hi again, fixed some dependency issues with mockito-core by updating its version to the latest. Also fixed some of the Findbug errors. It would make more sense to me to go over the remaining less critical findbug issues in a separate smaller pull request.

Please let me know your desire about the constructor.

akostadinov · 2017-12-13T18:23:22Z

I decided to use the less-efficient existing API of script-security plugin until the new one is merged. Functionality is the same and switching to the efficient API will be very easy to do once merged upstream. See commit use older API until newer is merged for details.

Now the only problem I see above is compiling with java 1.7. I think this is expected because maven-hpi-plugin has a target of java 1.8. This IMO shouldn't be a problem given we keep target 1.7 in logstash plugin. My suggestion would be to stop compiling the plugin with java 1.7 and only compile with 1.8+ going forward.

Alternatively, if you want to keep build support for 1.7, I figured parent plugin pom version could be set to 2.9 instead of the current 3.2.

Please let me know how do you prefer me to proceed.

jakub-bochenski · 2017-12-13T19:21:32Z

Alternatively, if you want to keep build support for 1.7, I figured parent plugin pom version could be set to 2.9 instead of the current 3.2.

I'm not sure how big a difference there is between 2.9 and 3.2, but in general I'd prefer to do the changes in smallest feasible increments. Just in case something breaks.

mwinter69 · 2017-12-13T19:41:48Z

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

@@ -207,8 +211,15 @@ public BuildData(Run<?, ?> build, Date currentTime, TaskListener listener) {
    }
  }

+  // ISO 8601 date format
+  public static DateFormat isoDateFormatter() {
+    return new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ssZ");


This is a potential performance bottleneck. Every time you create a new instance which means that the formatter needs to parse the string. Having just one instance as before which is reused should be faster

SimpleDateFormat is not thread safe, as pointed out by Findbugs :)

If we care about performance overhead the easiest replacement is https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/time/FastDateFormat.html

.. or this from JDK https://docs.oracle.com/javase/8/docs/api/java/time/format/DateTimeFormatter.html

I don't see DateTimeFormatter available on Java 7. Do you want the apache one or keep current approach. Btw if we use the apache commons formatter we will still change API compatibility as it is only a Format, not DateFormat.

To me it looks like apache commons formatter would be the immediate preferable choice. But there is another question. I see that the new DateTimeFormatter comming with Java8 is not inheriting Format at all. So perhaps BuildData class can provide a method to format some date instead of returning som formatter object. This will allow in the future to change implementation without breaking public API.

Interesting comparison http://optimizationtricks.blogspot.bg/2015/01/datetimeformat-comparison-java-vs-joda.html , please tell me.

re. Java 8: right I keep forgetting that it's still building on 7

re. format only: AFAIK we only use it to format, anyhow since 3.2 it supports parsing too

I'm fine with either Joda or FastDateFormat

akostadinov · 2017-12-13T19:59:16Z

@jakub-bochenski , 2.9 is from May 2016. 3.2 is from this month. I believe there are mostly differences in the support libraries, not for runtime but I don't know for sure. I can say that 3.2 brought better findbugs validation.

As you prefer I updated version to 2.9 to have 1.7 compilation succeed. We can update to 3.2+ later after some testing on jenkins vs java 7.

akostadinov · 2017-12-14T14:58:21Z

Updated to FastDateFormat from apache commons lang 2.x because I noticed it is already included in dependencies somehow. Also changed implementation to never expose the underlying formatter but only perform the formatting. This is IMO easier to support in the long run regardless of underlying implementation.

FYI I did a benchmark over the different approaches and there is no significant difference between FastDateFormat and Joda-Time.

akostadinov · 2017-12-14T21:58:04Z

FYI upstream is supporting only builds on java8 going forward. They suggest patching the travis build to stop doing java 7 builds:
jenkinsci/plugin-pom#65 (comment)

But this is for a future PR. Please let me know if you see any other necessary changes here.

jglick

A few technical problems.

More broadly, this seems risky since it will fundamentally clash with #18, which involves offloading delivery of most log lines from the master to agent JVM. OTOH that was merely a proof of concept and perhaps any production-quality Logstash support for Pipeline at scale would need to be written from scratch anyway.

(Can this kind of processing not be done after the fact BTW?)

jglick · 2017-12-15T15:46:22Z

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

  /**
   * Create a new {@link LogstashBuildWrapper}.
   */
  @DataBoundConstructor
+  public LogstashBuildWrapper(
+      @CheckForNull SecureGroovyScript secureGroovyScript


Rather use @DataBoundSetter.

jglick · 2017-12-15T15:47:44Z

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

+    binding = new Binding();
+    binding.setVariable("console", new BuildConsoleWrapper());
+
+    // not sure what the diff is compared to getClass().getClassLoader();


uberClassLoader would be able to load typed defined in any enabled plugin. LogstashScriptProcessor.class.getClassLoader() would be able to load only those types accessible at compile time in this plugin.

sounds like uberClassLoader gives more flexibility to script writers

it does, but then the scripts will be non portable -- you can have them working in one Jenkins instance but not another.
Security might be another concern, though the scripts need to be pre-approved anyway.

I'd suggest to do the more restrictive thing for now. It's easier to extend the possibilities later than restrict them

you can have them working in one Jenkins instance but not another

This is certainly a risk in any case.

Security might be another concern

Sandboxed scripts could only use whitelisted methods anyway.

jglick · 2017-12-15T15:48:23Z

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

+  @SuppressFBWarnings(
+    value="DM_DEFAULT_ENCODING",
+    justification="TODO: not sure how to fix this")
+  private void buildLogPrintln(Object o) throws IOException {


Whitelisting a private method makes no sense. If you intend for scripts to call it (directly), make it public. Since it seems to be called only indirectly from BuildConsoleWrapper, it does not need to be annotated.

I think this is a leftover from my first attempts. Will fix!

I'm removing the extra whitelist now. Just wondering how can I make the script this method directly? I couldn't see this possible.

jglick · 2017-12-15T15:49:46Z

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

+
+  /** Groovy binding for script execution */
+  @Nonnull
+  private final Binding binding;


Seems unsafe to keep this as a field. Initialize in process.

Why is it unsafe? The main idea is to allow executing the script always with the same binding. This would allow script to create properties for persisting data between executions. The way I understand your suggestion would not allow this. Or did I misunderstand?

Also, if we generate Binding in process(), that would mean creating a new object per each line of console output.

jglick · 2017-12-15T15:50:07Z

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

+  public JSONObject process(JSONObject payload) throws Exception {
+    binding.setVariable("payload", payload);
+    script.evaluate(classLoader, binding);
+    Object processedPayload = binding.getVariable("payload");


Just use the return value from evaluate.

This is a question that also a colleague raised and it was my initial implementation. But then I thought about writing example usage scripts and at least to me, the code to modify payload variable is shorter and more reliable.

Script author would need to always think what value is returned by different code paths. e.g. in a very simple use case author wants to add some additional properties to the message

if (payload["message"][0] =~ /whatever/) { payload["custom_prop"] = "custom_value" }

If we look at payload variable the above is enough. If we look at return value, one always has to add the line return payload. I don't insist on this. It might be only in my head, but IMO it will be a common annoyance to forget that return value.

I will change it if the return value is preferred.

My vote would be to use the return value.
Note: in groovy scripts the last statement's value is implicitly returned, so it's not as verbose.

I think we could make this work with some sensible defaults (e.g. returned String overrides message, returned map overrides the whole payload).

What I don't like about the current solution:

mutating the input binding

relying on null as magic value

return value is much more obvious to script readers

Thanks for the reply!

I will change to the return approach then [1].

wrt relying on null as magic value, how else do you suggest to signal backend to ignore some message?

wrt returned String overrides message, returned map overrides the whole payload - this will complicate backend IMO and provided script receives a Map/JSONObject as input, I'm not sure it will make things easier.

wrt mutating the input binding, not sure what exactly you mean with this. Are you supporting @jglick to create a new binding for each process() call? I can do this but then there is the question - how to allow persisting some data between script invocations. Perhaps introduce some Map variable (e.g. cache), where script can store some values for later? Let me know how you want to see it implemented [1].

Also would be very useful to advise whether this can be merged before #18 and does anybody has a plan for moving #18 forward (and what exactly needs to be fixed in #18 to begin with).

[1] I really don't care much as long as the necessary functionality is available.

This all all just ideas to improve the readability of the scripts. If you don't think it fits the intended use case please let me know.

how else do you suggest to signal backend to ignore some message?

How about we use the Null-Object pattern? bind something like 'SKIP' etc. to the script, then check for this exact value instead of null.

return SKIP

or

if (someCondition) SKIP

looks much clearer than

return null

his will complicate backend IMO and provided script receives a Map/JSONObject as input, I'm not sure it will make things easier.

IMO, I think it's OK (or even a feature) in loosely typed languages to provide different processing for different types.

My intention here is to enable a short way of transforming just the log message, e.g.

payload["message"][0].replace('_',' ')

introduce some Map variable (e.g. cache), where script can store some values for later

Actually I had something different in mind, but I like this idea. I think this could be done as an improvement later though (provided we mark the current behavior as experimental etc.)

Also would be very useful to advise whether this can be merged before #18 and does anybody has a plan for moving #18 forward

AFAIK it was a POC and there is no plan to move with it any time soon, but @jglick knows best.

jglick · 2017-12-15T15:50:33Z

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

+    script.evaluate(classLoader, binding);
+    Object processedPayload = binding.getVariable("payload");
+    if (processedPayload != null && !(processedPayload instanceof JSONObject)) {
+      throw new ClassCastException("script returned " + processedPayload.getClass().getName() + " instead of JSONObject");


Useless. The default ClassCastException would encode the same information.

Could you confirm that you mean to just remove this check and let line 110 throw it by itself? Sounds good to me, just want to make sure this is your proposal.

jglick · 2017-12-15T15:57:31Z

src/main/resources/jenkins/plugins/logstash/LogstashBuildWrapper/help-script.html

+<p>
+  Example script to filter out some console messages by pattern:
+  <pre><code>
+    if (payload && payload["message"][0] =~ /my needless pattern/) {


akostadinov · 2017-12-15T19:09:49Z

@jglick , thanks a lot for chiming in. Besides your existing comments, would you advice about the DM_DEFAULT_ENCODING suppressions? Is there any better way to handle these or we are expected to use default encoding, thus we should just keep them suppressed?

akostadinov · 2017-12-16T07:18:55Z

@jglick , I missed your comment about #18. I don't think this pull request conflicts it much. I'm +1 for moving log processing to slaves [1]. So if you have time to move #18 forward, I'll add groovy to the result. Or if you can't do that for the time being, I think it shouldn't be so hard to account in #18 for this one.
You say that #18 is not production quality. It's really deep water for me to tell what needs to be improved and how. This is my first major jenkins plugin contribution and it will cost me too much time even to understand what are you doing in #18.

[1] Although it might be a problem if logstash is in some VPN while slaves are connected from public clouds for example. Could processing be done on slaves but data sent through master? Maybe unnecessary complication, just a thought.

jakub-bochenski · 2017-12-20T19:59:23Z

src/main/resources/jenkins/plugins/logstash/LogstashBuildWrapper/help-script.html

+<p>
+  The script will be executed once per each line of build console output with the
+  variable <code>payload</code> set to a JSON payload with the "message" key being
+  an array with one string element - the current line.


actually why a 1 element array?

This is the format of the generated message JSON. At least when using the wrapper. User is free to add/modify lines. In the post-build action there are more elements.

OK maybe we could clarify here that the whole payload as would be sent to the backend is available here. One could read the above as payload being a map with a single "message" key

I tried to clarify the text. Let me know if it is better now. Or if you provide me with better wording, I would gladly update with it. I'm not a native english speaker.

jakub-bochenski · 2017-12-20T20:04:40Z

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

+  public JSONObject process(JSONObject payload) throws Exception {
+    binding.setVariable("payload", payload);
+    script.evaluate(classLoader, binding);
+    Object processedPayload = binding.getVariable("payload");


This all all just ideas to improve the readability of the scripts. If you don't think it fits the intended use case please let me know.

how else do you suggest to signal backend to ignore some message?

How about we use the Null-Object pattern? bind something like 'SKIP' etc. to the script, then check for this exact value instead of null.

return SKIP

or

if (someCondition) SKIP

looks much clearer than

return null

his will complicate backend IMO and provided script receives a Map/JSONObject as input, I'm not sure it will make things easier.

IMO, I think it's OK (or even a feature) in loosely typed languages to provide different processing for different types.

My intention here is to enable a short way of transforming just the log message, e.g.

payload["message"][0].replace('_',' ')

introduce some Map variable (e.g. cache), where script can store some values for later

Actually I had something different in mind, but I like this idea. I think this could be done as an improvement later though (provided we mark the current behavior as experimental etc.)

Also would be very useful to advise whether this can be merged before #18 and does anybody has a plan for moving #18 forward

AFAIK it was a POC and there is no plan to move with it any time soon, but @jglick knows best.

jakub-bochenski · 2017-12-20T20:09:24Z

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

    when(mockBuild.getRootBuild()).thenReturn(mockBuild);
    when(mockBuild.getBuildVariables()).thenReturn(Collections.emptyMap());
    when(mockBuild.getSensitiveBuildVariables()).thenReturn(Collections.emptySet());
    when(mockBuild.getEnvironments()).thenReturn(null);
    when(mockBuild.getAction(AbstractTestResultAction.class)).thenReturn(mockTestResultAction);
-    when(mockBuild.getLog(0)).thenReturn(Arrays.asList());
+    // when(mockBuild.getLog(0)).thenReturn(Arrays.asList());


please don't leave commented out code, just delete

removed all occurrences of commented out lines that I could spot (3), thanks

see jenkinsci/script-security-plugin#169

akostadinov · 2017-12-21T13:25:55Z

@jakub-bochenski , I missed your other suggestions, now I see them. I see you find the SKIP approach better. I'll try it out, looks good. What should I do with a null return though? Should I handle it too?

wrt cache variable, given you find such approach better, then I can implement it right away, not big deal. I just wanted to avoid creating new objects as much as possible, that's why I wanted to reuse the Binding. Just please confirm that you prefer the cache variable approach.

wrt IMO, I think it's OK (or even a feature) in loosely typed languages to provide different processing for different types.. The issue is actually that we don't know whether user has modified the payload or not. If we want to be 100% reliable, when we get a string, then we should generate new payload and stuff the string into it instead of modifying the already existing payload. This is IMO too much of an overhead.
On the other hand, if you are fine with the idea that user may shoot themselves in the foot if they modify payload and at the same time return just a string, then I'll be glad to do your suggested modification as it would be a neat way for simple line modification.

Inspired by above suggestion another interesting approach would be to also allow simple filtering by returning a matcher or a boolean. e.g.

payload["message"][0] =~ /my pattern/

or

payload["message"][0].contains("my partial string")

or even a pattern

/my pattern/

We can also bind the message as a separate variable message to avoid the need to access the full payload when user does not need to. I assume accessing message would be the most common use case.

In all situations though we need to accept the risk of somebody clueless shooting himself in the foot by modifying payload and then returning something else. Let me know.

jakub-bochenski · 2017-12-21T13:39:26Z

I just wanted to avoid creating new objects as much as possible, that's why I wanted to reuse the Binding. Just please confirm that you prefer the cache variable approach.

I'm not sure what was the original @jglick point about that. I was just commenting from general principles.
I do prefer cache for the reasons listed above.

I don't think we should worry about creating new objects in Java. Modern GCs hanlde short-lived objects just fine. I'd only worry if the objects have long initialization times (expensive operations in constructor)

jakub-bochenski · 2017-12-21T13:42:31Z

On the other hand, if you are fine with the idea that user may shoot themselves in the foot if they modify payload and at the same time return just a string, then I'll be glad to do your suggested modification as it would be a neat way for simple line modification.

I had that in mind in the previous comment about mutating state.

My suggestion here is:

only take into account return value (regardless of how we process it later)
wrap the payload in an unmodifiable wrapper (like unmodifiableMap); failing during the build seems better than having to check the logging backend to discover data is missing

jakub-bochenski · 2017-12-21T13:44:22Z

We can also bind the message as a separate variable message to avoid the need to access the full payload when user does not need to. I assume accessing message would be the most common use case.

good idea, I think this would be useful weather we go with the other changes or no

jakub-bochenski · 2017-12-21T13:47:09Z

Inspired by above suggestion another interesting approach would be to also allow simple filtering by returning a matcher or a boolean. e.g.

+1 to that.
I would advise some caution only because once we add something it will be hard to remove. I'd support only a few cases in the initial release and then gradually add more.

jakub-bochenski · 2017-12-21T14:02:41Z

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

@@ -85,6 +90,9 @@ public boolean configure(StaplerRequest req, JSONObject formData) throws FormExc
    }

    @Override
+    @SuppressFBWarnings(
+      value="NP_PARAMETER_MUST_BE_NONNULL_BUT_MARKED_AS_NULLABLE",
+      justification="TODO: investigate")


can't we just leave those cases unsuppressed? or is the build setup to fail in that case?

build fails

ok, I think we should disable this on maven level then -- just make it emit warnings for now

let me know if you need help with this

project automated CI will will show a failure. Is there a way to make it not fail but just warn?

this should be set somewhere in maven and be overridable -- let me check

we have the FB issues fixed in #43 and related PRs #44 #45 #46
maybe you can integrate those into your PR instead?

Will look into that once I catch up with the holiday pile up of tasks.

jakub-bochenski · 2017-12-21T14:07:23Z

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

@@ -85,6 +90,9 @@ public boolean configure(StaplerRequest req, JSONObject formData) throws FormExc
    }

    @Override
+    @SuppressFBWarnings(
+      value="NP_PARAMETER_MUST_BE_NONNULL_BUT_MARKED_AS_NULLABLE",
+      justification="TODO: investigate")
    public ToolInstallation newInstance(StaplerRequest req, JSONObject formData) throws FormException {


Req is marked as nullable in the interface, so apparently we should handle this gracefully.

I just suppressed the warning, I don't think I touched anything about this. Good to go after all suppression later and have them fixed.

jakub-bochenski · 2017-12-21T14:12:00Z

src/main/resources/jenkins/plugins/logstash/LogstashBuildWrapper/help-script.html

+  configured logstash backend. The following variables are available during execution:
+  <ul>
+    <li><strong>payload</strong> - JSONObject contaning the payload to be persisted.</li>
+    <li><strong>console</strong> - a class to output messages to build log without persisting them. It also works in a sandbox: <code>console.println("my logged message")</code>.</li>


Since pipeline is using an echo statement maybe it would be nice to use the same (at least make it look the same).

echo "my logged message"

I don't know how to do this. I didn't find any way to add a method to the binding. And using org.codehaus.groovy.runtime.MethodClosure to create a closure property in the binding didn't work either with some script security issue (I tried this already). Maybe my groovy shell foo is lacking. Any ideas would be welcome.

Why not bind groovy.lang.Closure?

If that doesn't work then lets drop it, it's only nice to have

FYI doing echo "some string" in script resulted in

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

While echo.call "some string" worked. I couldn't find a way to get around that.

jakub-bochenski · 2017-12-21T14:14:56Z

src/main/resources/jenkins/plugins/logstash/LogstashBuildWrapper/help-script.html

+  This means that variables can be saved between script invocations.
+</p>
+<p>
+  At the end of the build a payload == null will be submitted. You can use this to


Some other ideas on how to not depend on null:

bind a special marker to do payload == DONE instead

bind a status variable e.g. if (buildFinished)

accept a completely separate finalizer script

jakub-bochenski · 2017-12-21T14:16:34Z

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

+    inOrder.verify(mockBuffer).close();
+
+    // Verify results
+    assertEquals("Results don't match", "", buffer.toString());


That message doesn't really add anything over what the matcher would report. Can we remove it?

Actually I think it would be better to use hamcrest here:

assertThat( buffer, hasToString(""))

http://hamcrest.org/JavaHamcrest/javadoc/1.3/org/hamcrest/object/HasToString.html

akostadinov · 2017-12-21T16:13:54Z

wrap the payload in an unmodifiable wrapper (like unmodifiableMap); failing during the build seems better than having to check the logging backend to discover data is missing

This will break the other use cases like mine and for #40 where we want to add/remove other fields to the payload.
At the same time I think that we need as little overhead as possible, because things will be performed once for each line in the log and that can be a lot. We casually have Megabytes of console log. And in fact I'm looking at splitting these up into chunks that make logical sense with attached additional metadata to them (if I didn't explain my use case yet).

IMO we can't protect users from every possible mistake. Can we let users modify the JSON and return it for the sake of simplicity? Perhaps explain this gotcha in the help. Use case like:

payload['my_custom_field'] = "some value"
return payload;

jakub-bochenski · 2017-12-22T19:36:13Z

This will break the other use cases like mine and for #40 where we want to add/remove other fields to the payload.

You can still make a copy and modify that. If you are worried about the overhead of doing a copy then I think it's OK to leave it mutable.

stale

jglick · 2018-01-10T20:27:12Z

pom.xml

+    <dependency>
+      <groupId>org.jenkins-ci.plugins</groupId>
+      <artifactId>script-security</artifactId>
+      <version>1.37</version>


I thought you were going to use jenkinsci/script-security-plugin#169 here, so we can verify that the proposed API actually works for its supposed purpose? If so, you would need to deploy a snapshot of the upstream PR, then change this to a timestamped snapshot version, so CI can verify this PR.

@jglick , I have verified the API works locally. Would you point me at how to deploy a timestamped snapshot?

I think @jglick meant to put a timestamped snapshot dependency here. If you point me to the artifact you need I think I can make the change.

EDIT: no apparently we also need to deploy it first. Still I think I could help with that but please point me to the exact thing you need.

Basically we need jenkinsci/script-security-plugin#169 built and deployed. Also we need 747a0bc reverted here so that the new API is used. Better let me first do little refactoring of script security pull request and then test it. I just have so much other work since new year. Hope to get back to this next week.

jglick · 2018-01-10T20:31:22Z

I missed your comment about #18. I don't think this pull request conflicts it much.

Well, “conflict” to the extent that you could choose to do either remote logging, or script processing, but not both—scripts as implemented here would be master-only.

Anyway, do not worry about it much—as I said, it is just as likely any remote logging plugin would be written from scratch, and may not be based on Logstash at all.

akostadinov · 2018-01-10T20:51:42Z

@jglick , and what shall be done to allow both? If nobody can work on the remote logging, then I guess what we have should be good enough. On the other hand I'm willing to change things for a better plugin but I'm very inexperienced with Jenkins. I don't want to delay the feature too much. But I plan to be in Brussels between 1st and 4th if you can lend a hand.

akostadinov · 2018-03-20T17:36:41Z

Wow, thank you for merging. I'm planning to move the more efficient version forward but I'm stuck with other priority work. This merge would definitely help though.

jakub-bochenski · 2018-03-20T18:04:34Z

@akostadinov wait, I didn't intend to merge this into master - I just wanted to resolve the conflicts on your branch

I'll have to undo that as the build is not passing

jakub-bochenski · 2018-03-20T18:10:25Z

@akostadinov @jglick @oleg-nenashev sorry about the mixup
I've overwritten the master branch, but I'm afraid that this PR will stay as "merged"

I've put my attempt at resolving the conflicts into #56
@akostadinov unfortunately I've botched something -- 2 tests are failing, can you maybe take a look?

akostadinov · 2018-03-20T19:16:17Z

ah, it's fine. Thank you for helping with merging conflicts. Issue seem to be some minor things. I really hope to get back to this next week.

This was referenced Dec 12, 2017

Ability to send custom data to logstash #39

Closed

Parent pom upgrade #17

Closed

[JENKINS-34891] Fixed dependency to mask-passwords #14

Closed

exception without mask-passwords plugin

f662b45

akostadinov force-pushed the master branch from 36d07d3 to 5927282 Compare December 12, 2017 23:14

oleg-nenashev requested changes Dec 12, 2017

View reviewed changes

akostadinov force-pushed the master branch 2 times, most recently from 47f2bbe to 0f59cf5 Compare December 13, 2017 15:36

jakub-bochenski mentioned this pull request Dec 13, 2017

properly get displayname of node #38

Merged

mwinter69 reviewed Dec 13, 2017

View reviewed changes

saner Jenkins version requirement

d4f0396

akostadinov force-pushed the master branch from 0f59cf5 to 09f7cd0 Compare December 13, 2017 19:50

akostadinov force-pushed the master branch from 09f7cd0 to d65ba8b Compare December 14, 2017 14:31

jglick previously requested changes Dec 15, 2017

View reviewed changes

jglick reviewed Dec 15, 2017

View reviewed changes

akostadinov force-pushed the master branch from d65ba8b to 3751e6d Compare December 18, 2017 13:46

jakub-bochenski reviewed Dec 20, 2017

View reviewed changes

akostadinov added 3 commits December 21, 2017 15:03

support custom log lines processing via script

9c57498

fix dep and FindBug issues

f69b2de

use older API until newer is merged

747a0bc

see jenkinsci/script-security-plugin#169

akostadinov force-pushed the master branch from 3751e6d to 747a0bc Compare December 21, 2017 13:04

jakub-bochenski reviewed Dec 21, 2017

View reviewed changes

jakub-bochenski mentioned this pull request Dec 29, 2017

findbugs: fix issues in BuildData #44

Merged

jglick reviewed Jan 10, 2018

View reviewed changes

temp use snapshot

321d77a

jakub-bochenski merged commit 321d77a into jenkinsci:master Mar 20, 2018

jakub-bochenski mentioned this pull request Mar 20, 2018

support custom log lines processing via script #56

Closed

akostadinov mentioned this pull request Jun 21, 2018

allow scripted changes to logged data #71

Open

support custom log lines processing via script #40

support custom log lines processing via script #40

Conversation

akostadinov commented Dec 12, 2017

oleg-nenashev 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

akostadinov commented Dec 13, 2017

akostadinov commented Dec 13, 2017

jakub-bochenski commented Dec 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akostadinov Dec 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akostadinov commented Dec 13, 2017

akostadinov commented Dec 14, 2017

akostadinov commented Dec 14, 2017

jglick 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

Choose a reason for hiding this comment

jakub-bochenski Dec 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakub-bochenski Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akostadinov Dec 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akostadinov commented Dec 15, 2017

akostadinov commented Dec 16, 2017

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 Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akostadinov commented Dec 21, 2017 • edited Loading

jakub-bochenski commented Dec 21, 2017

jakub-bochenski commented Dec 21, 2017

jakub-bochenski commented Dec 21, 2017

jakub-bochenski commented Dec 21, 2017 • 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

jakub-bochenski Dec 22, 2017 • 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

akostadinov Dec 13, 2017 •

edited

Loading

jakub-bochenski Dec 19, 2017 •

edited

Loading

jakub-bochenski Dec 20, 2017 •

edited

Loading

akostadinov Dec 15, 2017 •

edited

Loading

jakub-bochenski Dec 20, 2017 •

edited

Loading

akostadinov commented Dec 21, 2017 •

edited

Loading

jakub-bochenski commented Dec 21, 2017 •

edited

Loading

jakub-bochenski Dec 22, 2017 •

edited

Loading

jakub-bochenski Dec 21, 2017 •

edited

Loading

jakub-bochenski Jan 12, 2018 •

edited

Loading