move configuration from ToolInstallation to GlobalConfiguration #43

mwinter69 · 2017-12-27T21:18:37Z

The plugin used a ToolInstallation for configuration. But logstash is
not a tool that can be installed. Instead this should be a
GlobalConfiguration.
The indexers have different configuration requirements. Host and port
are common, but things like username are not required everywhere. Also
the previous implementation was totally unflexible. In order to add a
new indexer one has to modify the sources. By configuring the indexers
via an extension point we make it easily possible to have individual
configuration jelly for each type. Adding a new indexer is just adding
new classes. So it can be even moved to a separate plugin.

use the executor of the build to get the node. builtOn is set too late for us to get the node and we end up using master

properly get the node when BuildData is initilaized from a pipeline pump mockito to latest version add integration test, that use Jenkins test harness

the plugin used a ToolInstallation for configuration. But logstash is not a tool that can be installed. Instead this should be a GlobalConfiguration. The indexers have different configuration requirements. Host and port are common, but things like username are not required everywhere. Also the previous implementation was totally unflexible. In order to add a new indexer one has to modify the sources. By configuring the indexers via an extension point we make it easily possible to have individual configuration jelly for each type. Adding a new indexer is just adding new classes. So it can be even moved to a separate plugin. Many of the fields in the plugin were public or package private. They have been made private or protected with corresponding getters and setters and direct calls to the fields replaced. Get the charset of a run and use it whenever we need to convert the String to a byte array Use DateFormatter from apache common lang to avoid potential threading problems.

properly get displayname of node (jenkinsci#38)

use Jenkins 2.7.4 instead of 2.60.3 fix problem with cast that is requried with java 7

jakub-bochenski · 2017-12-28T14:18:58Z

Thanks, that's great.

The indexers have different configuration requirements. Host and port
are common, but things like username are not required everywhere. Also
the previous implementation was totally unflexible.

Can you check #41? We are adding a new global config field there, but hopefully this PR might supersede it?

jakub-bochenski · 2017-12-28T14:22:44Z

Many of the fields in the plugin were public or package private. They
have been made private or protected with corresponding getters and
setters and direct calls to the fields replaced.

Get the charset of a run and use it whenever we need to convert the
String to a byte array

Use DateFormatter from apache common lang to avoid potential threading
problems.

Fixed all findbugs warnings.

Would it be possible to send each of those as a separate PR? It will make it much easier to review and integrate (e.g. hopefully we can merge the purely refactoring changes immediatelly)

Use FastDateFormat from apache which is thread safe Avoid possible NPE in Node detection Avoid possible NPE in result (previous logic already avoided it but findbugs still claimed it to be a problem) Node detection is same for pipeline and freestyle so move it to initData method

mwinter69 · 2017-12-28T21:51:13Z

OK will split this in several PRs

jakub-bochenski · 2017-12-29T15:48:04Z

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

+    extends AbstractDescribableImpl<LogstashIndexer<?>>
+    implements ExtensionPoint, ReconfigurableDescribable<LogstashIndexer<?>>
+{
+  protected String host;


I'm not sure, but maybe it would be better to extract those fields into a separate data object?
It seems cleaner to push the check in shouldRefreshInstance() into the equals() method instead of overriding shouldRefreshInstance() directly.

But we're not comparing the same objects in this method so the equals() method is not the right place. Currently there is a separation between the configuration and the actual dao that does the push to the indexer. The configuration class must decide if it should create a new dao instance or return the existing one which is done in the shouldRefreshInstance().

What I don't like here is that LogstashIndexer is an abstract factory for the dao while also being the model for the configuration view (jelly). My intuition suggest extracting the model into a separate entity.

Maybe let me send you a patch to illustrate (as it might be easier than explaining) and you can revert it if you don't like it.

@mwinter69 can you check ae96695 ?
It's only a rough draft but I think it demonstrates the idea (e.g. I'd rather have the data object immutable than cloning it).

Please excuse me for the messed up formatting, I accidentally formatted the whole Syslog.java

I think this makes the whole thing with stapler and configuration more complex. Not sure if we have to make the Data object then also a describable so that stapler can auto configure it when global configuration is saved.
We could also move the comparison to the GlobalConfiguration class.
A third option would be to make the daos directly the thing that is configured.

With your explanation I think it's OK to leave the code as is. It's still better than previous one.

We could also move the comparison to the GlobalConfiguration class.

Can you explain what you mean by this?

Take a look at https://github.com/mwinter69/logstash-plugin/tree/configuration2
One effect of stapler is that when you bind a request to the configuration you always get a new instance of the logstashindexer. Thats why I had this check and implemented the reconfigure.
In this branch I replaced this by comparing (with equals) the newly configured instance with the active instance. If they are different, the active is replaced with the new one.

I like configuration2 more since we are comparing instances of LogstashIndexer instead of comparing LogstashIndexer and a LogstashDao.

…into jenkinsci-master

reduce visibility of fields (jenkinsci#46)

update javadoc to get more information for developers that want to extend the plugin. Update the readme Use the not deprecated junit runner of mockito

jakub-bochenski · 2018-01-03T10:52:26Z

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

+   * @return {@link AbstractLogstashIndexerDao} instance
+   */
+  @Nonnull
+  public final T getInstance()


Do we need to synchronize it? I can't find anything via a quick google

good question. Potentially after a reconfiguration or initially we might have 2 builds running into the if at the same time. Probably it will not hurt to synchronize. The worst that happens is that 2 builds have 2 different daos with the same configuration. Will this be a problem? We will already have different daos active when configuration is changed (the builds that were running when configuration changed and builds started after change).

Not at the moment, but I think it would be nice to enable a close() method on the Daos later to let them use long living connections etc.

Another problem is somebody calling setHost() concurrently with getInstance() -- those are public methods after all.

Maybe it's covered by creating a new instance

when you bind a request to the configuration you always get a new instance of the logstashindexer

but then I don't actually understand how can caching the Dao instance in the LogstashIndexer work?

it is cached because I use the reconfigure instead of the default behaviour of staper to create new instance. But this question is not relevant when we switch to the change in my configuration2 branch.

jakub-bochenski · 2018-01-03T10:53:25Z

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

+  protected boolean shouldRefreshInstance()
+  {
+    return super.shouldRefreshInstance() ||
+        !instance.getMessageFormat().equals(messageFormat);


we are not checking the protocol -- is this intentional?

The protocol is currently not used in the dao. Maybe we can remove this completely. Or we also implement the tcp protocol.

There were attempts to implement TCP in the past: #26

I'd either remove the field or use it in comparison to avoid future confusion.

will add it to comparison. We should also implement the tcp protocol I think, just for completeness

jakub-bochenski · 2018-01-03T10:54:50Z

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

@@ -37,7 +37,7 @@
 * @since 1.0.0
 */
 public interface LogstashIndexerDao {
-  static enum IndexerType {
+  public static enum IndexerType {


interface members are public by default, I'd prefer to not restate default modifiers

jakub-bochenski · 2018-01-03T10:57:54Z

src/main/resources/jenkins/plugins/logstash/LogstashInstallation/global.jelly

-      </f:entry>
-    </f:advanced>
-  </f:section>
+<!-- We need to keep this file in order to prevent that in the UI we get the option to configure a Logstash Installation -->


How about leaving some explanatory text here, e.g. "Configuration moved to Global Configuration"? Bonus points for a link :)

I think it should be enough to document this on the wiki page.
btw I suggest to upgrade the version to 2.0. And maybe we find a way to show that plugin changed in a way that when you go back to previous version you have to manually adjust configuration (this is possible somehow).

I know they should check the wiki page, but this makes for less frustration.

Can we leave a short message here with a link to the wiki page then?

will add a link to global configuration page

jakub-bochenski · 2018-01-03T11:01:50Z

src/main/resources/jenkins/plugins/logstash/configuration/ElasticSearch/help-host.html

@@ -1,4 +1,4 @@
 <div>
  <p>The host name or IP address of the indexer to send log data to.<br>
-  ELASTICSEARCH: Also specify scheme. Example: "https://myserver". </p>
+  Also specify scheme. Example: "https://myserver". </p>


can we add a separate scheme field instead?

actually what would be even better is to have a single field accepting a full URL instead. Do you think it's possible?

Then we could also remove the key, as this is just added to the url

jakub-bochenski · 2018-01-03T11:08:24Z

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

+  }
+
+  @Test
+  public void test_redisMigration()


can we get rid of the test_ prefixes? It's not needed in JUnit4

jakub-bochenski · 2018-01-03T11:17:39Z

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

+    implements ExtensionPoint, ReconfigurableDescribable<LogstashIndexer<?>>
+{
+  protected String host;
+  protected int port;


On a different note: maybe we should push the host and port up?
MemoryDao is one obvious example that doesn't use those, but I think it might be cleaner code in general.
We could have another helper abstract class to avoid copying this for all the indexer types

If we replace for elastic search the host and port with a url this might make sense

rename junit test methods synchronize getting the dao instance to avoid problems. deprecate enums

jakub-bochenski · 2018-01-03T14:15:29Z

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

+    // when we bind the stapler request we get a new instance of logstashIndexer
+    staplerRequest.bindJSON(this, json);
+
+    if (logstashIndexer != null && !logstashIndexer.equals(activeIndexer))


Objects.equals() seems shorter

introduce an abstract host based indexer that serves as base for most indexers. Elastic search uses a uri approach where we need scheme, host, port and url path, so we allow to configure a uri instead.

jakub-bochenski · 2018-01-04T17:43:21Z

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

+            {
+              uri = (new URIBuilder(descriptor.getHost()))
+                  .setPort(descriptor.getPort())
+                  .setPath("/" + descriptor.getKey()).build();


not sure if this will work properly for keys that already start with /
does this UriBuilder collapse consecutive /?

This is exactly the same behaviour as it was before in the ElasticSearchDao

jakub-bochenski · 2018-01-04T17:44:43Z

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


  @DataBoundConstructor
-  public ElasticSearch()
+  public ElasticSearch(String uri) throws URISyntaxException


Why are we changing the constructor instead of using a @DataBoundSetter?

When the uri is not valid it will fail to save the configuration when it is checked in the constructor.
When using a DataBoundSetter, you can save the globalconfiguration with invalid uri

jakub-bochenski · 2018-01-04T17:47:51Z

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

      }

-      return FormValidation.ok();
+    if(uri.getPort() == -1) {


Why not let users rely on default port?
https://logstash.internal/index looks better than https://logstash.internal:443/index

jakub-bochenski · 2018-01-04T17:50:01Z

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

    }

+    if(StringUtils.isBlank(uri.getPath()) || uri.getPath().trim().matches("^\\/+$")) {


I have mixed feelings here.
It's obviously useful for 99% of usages, but probably somebody somewhere with some weird Apache/modrewrite in front will want to use an empty path.

Can we make this into a warning instead?

before it was also a must to have a non empty key

jakub-bochenski · 2018-01-04T17:51:33Z

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

+   * @param uri
+   * @throws IllegalArgumentException when one of scheme, port or path
+   */
+  public static void validateUri(URI uri) throws IllegalArgumentException


since we ignore those I'd add a check enforcing empty username and password (in authority part)

jakub-bochenski · 2018-01-04T17:52:08Z

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

  }

-  @DataBoundSetter
-  public void setKey(String key)
+  public void setUri(String value) throws URISyntaxException


minor; for sake of consistency can we make setter accept URI?

With URI stapler is not able to autoconvert the String to an URI. I will switch to URL as persistence here. Then I can also use a DataBoundSetter that accepts a URL

jakub-bochenski · 2018-01-04T17:52:57Z

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

+import org.kohsuke.stapler.DataBoundConstructor;
+import org.kohsuke.stapler.DataBoundSetter;
+
+public class HostConfiguration


I can't find any references to that class, am I missing something?

jakub-bochenski · 2018-01-04T17:57:06Z

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

+/**
+ * Abstract data access object for Logstash indexers.
+ *
+ * @author Rusty Gerard


I see some tasty copy-pasta here. Unless you are an alter ego of mr. Gerard?

Please just remove the @author tags, I think they are useless in a world with git blame

jakub-bochenski · 2018-01-04T17:58:11Z

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

+  {
+    URI uri = (new URIBuilder("localhost:8000")).setPath("/logstash").build();
+
+    //System.out.println(uri.toURL());


commented out code

actually it's weird FB isn't flagging this

forgot to delete this main class

stapler is not able to autconvert String to URI add more TODOs

jakub-bochenski · 2018-01-10T14:40:31Z

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

    // Clean up the the logstash log file
    try {
+      File file = new File(logfile);


minor: it's better to use http://junit.org/junit4/javadoc/4.12/org/junit/rules/TemporaryFolder.html

That was a try to get this test running in Eclipse. It always fails in Eclipse for me, thought it was due to missing folders, but didn't help. I'll remove that again.

jakub-bochenski · 2018-01-10T14:42:50Z

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

    } catch (IllegalArgumentException e) {
-      assertEquals("Wrong error message was thrown", "elastic index name is required", e.getMessage());
-      throw e;
+      assertEquals("Wrong error message was thrown", "java.net.MalformedURLException: unknown protocol: localhost", e.getMessage());


minor: http://junit.org/junit4/javadoc/4.12/org/junit/rules/ExpectedException.html looks like a better idea here

I consider using ExpectedException as a bad practice (see also https://artemzin.com/blog/test-expected-badpracticeexception/)
Why rethrow when we already catched it and checked the message is what we expect.

Yeah, but I was talking about the ExpectedException rule

@Rule public ExpectedException thrown= ExpectedException.none(); // ... thrown.expect(NullPointerException.class); thrown.expectMessage("happened"); throw new NullPointerException("What happened?");

jakub-bochenski · 2018-01-10T18:19:25Z

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

+  public LogstashConfiguration()
+  {
+    load();
+    if (logstashIndexer != null)


Why not always assign? I don't see how activeIndexer could be non-null here

It can be null when you install the plugin. But the activeIndexer being null is not a problem

jakub-bochenski · 2018-01-10T18:20:37Z

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

+  {
+    // when we bind the stapler request we get a new instance of logstashIndexer
+    staplerRequest.bindJSON(this, json);
+    if (!Objects.equals(logstashIndexer, activeIndexer))


can you add further comment explaining that we check for equality because Indexer instance is a holder for Dao

jakub-bochenski · 2018-01-10T18:23:59Z

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

+    }
+    // String comparison is not optimal but comparing the urls directly  is
+    // criticized by findbugs as being a blocking operation
+    else if (!url.toString().equals(other.url.toString()))


minor: I'd suggest to have an URI field instead and do an URL -> URI conversion in setter

jakub-bochenski · 2018-01-10T18:24:25Z

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

+    }
+  }
+
+  public static void main(String[] args) throws MalformedURLException, URISyntaxException


leftover test method?

jakub-bochenski · 2018-01-10T18:24:42Z

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

+    @Override
+    public int getDefaultPort()
+    {
+      return 9300;


umm.. isn't ES default port 9200?

yes but with URI, we don't need this at all.

jakub-bochenski · 2018-01-10T18:26:04Z

LGTM except for the minor issues listed above.

I think I should do another release, and then we can merge it and bump the major version.

URI can be easier compared some minor changes in tests

mwinter69 added 6 commits December 11, 2017 22:57

properly get displayname of node

371e27b

use the executor of the build to get the node. builtOn is set too late for us to get the node and we end up using master

displayname of node in pipeline

67bc346

properly get the node when BuildData is initilaized from a pipeline pump mockito to latest version add integration test, that use Jenkins test harness

Merge branch 'master' into configuration

faf30cd

Merge pull request #1 from jenkinsci/master

6b4c6d7

properly get displayname of node (jenkinsci#38)

jenkins and java

333ca41

use Jenkins 2.7.4 instead of 2.60.3 fix problem with cast that is requried with java 7

jakub-bochenski mentioned this pull request Dec 28, 2017

Use mime type from config field in Elasticsearch indexer while posting HTTP request #41

Merged

jakub-bochenski reviewed Dec 29, 2017

View reviewed changes

mwinter69 added 6 commits December 29, 2017 23:58

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

45a03ac

…into jenkinsci-master

Merge branch 'jenkinsci-master'

59d1a77

Merge pull request #3 from jenkinsci/master

96deff1

reduce visibility of fields (jenkinsci#46)

Merge branch 'master' into configuration

379c5e4

javadoc and readme

1321104

update javadoc to get more information for developers that want to extend the plugin. Update the readme Use the not deprecated junit runner of mockito

remove unnecessary dependency to workflow-step-api

4974276

jakub-bochenski mentioned this pull request Jan 2, 2018

support custom log lines processing via script #40

Merged

do the comparison if something has changed in the configuration class

227611b

jakub-bochenski reviewed Jan 3, 2018

View reviewed changes

mwinter69 added 3 commits January 3, 2018 14:06

incorporate feedback

d602a6e

rename junit test methods synchronize getting the dao instance to avoid problems. deprecate enums

Merge branch 'configuration2' into configuration

7340194

adjust link to plugin page

6116410

jakub-bochenski reviewed Jan 3, 2018

View reviewed changes

jenkinsci deleted a comment from mwinter69 Jan 3, 2018

mwinter69 added 3 commits January 3, 2018 21:00

simplify equals

ac70407

host based indexers vs uri

87c93cb

introduce an abstract host based indexer that serves as base for most indexers. Elastic search uses a uri approach where we need scheme, host, port and url path, so we allow to configure a uri instead.

add some todos

f021a43

jakub-bochenski reviewed Jan 4, 2018

View reviewed changes

use URL instead of URI

fdc79cc

stapler is not able to autconvert String to URI add more TODOs

jakub-bochenski reviewed Jan 10, 2018

View reviewed changes

mwinter69 added 3 commits January 10, 2018 22:32

use URI as persistence for elastic search url

dac27f1

URI can be easier compared some minor changes in tests

fix test

f2c4a24

use ExpectedException rule

add8146

mwinter69 mentioned this pull request Jan 28, 2018

Add support for Elasticsearch pipelines #49

Closed

Merge branch 'master' into configuration

d28cab4

jakub-bochenski merged commit c014184 into jenkinsci:master Jan 30, 2018

mwinter69 deleted the configuration branch January 31, 2018 01:03

		}

		if(StringUtils.isBlank(uri.getPath()) \|\| uri.getPath().trim().matches("^\\/+$")) {

move configuration from ToolInstallation to GlobalConfiguration #43

move configuration from ToolInstallation to GlobalConfiguration #43

Conversation

mwinter69 commented Dec 27, 2017 • edited Loading

jakub-bochenski commented Dec 28, 2017

jakub-bochenski commented Dec 28, 2017

mwinter69 commented Dec 28, 2017

Choose a reason for hiding this comment

mwinter69 Jan 1, 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

jakub-bochenski Jan 3, 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

mwinter69 Jan 3, 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

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

mwinter69 Jan 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwinter69 Jan 4, 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

jakub-bochenski Jan 4, 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

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 Jan 10, 2018

mwinter69 commented Dec 27, 2017 •

edited

Loading

mwinter69 Jan 1, 2018 •

edited

Loading

jakub-bochenski Jan 3, 2018 •

edited

Loading

mwinter69 Jan 3, 2018 •

edited

Loading

mwinter69 Jan 4, 2018 •

edited

Loading

mwinter69 Jan 4, 2018 •

edited

Loading

jakub-bochenski Jan 4, 2018 •

edited

Loading