Skip to content

Add: ElasticSearch backend listener #615

Closed
anthonygauthier wants to merge 1 commit intoapache:masterfrom
anthonygauthier:master
Closed

Add: ElasticSearch backend listener #615
anthonygauthier wants to merge 1 commit intoapache:masterfrom
anthonygauthier:master

Conversation

@anthonygauthier
Copy link

@anthonygauthier anthonygauthier commented Sep 2, 2020

Description

After 3 years of developing and maintaining the ElasticSearch Backend Listener plugin for JMeter, I believe it's time for it to be integrated to the main codebase.

  • Added org.apache.jmeter.visualizers.backend.elasticsearch package
  • Added the following classes:
    • ElasticsearchBackendClient.java
    • ElasticsearchMetric.java
    • ElasticsearchMetricSender.java
    • ElasticsearchRequests.java
  • Added unit tests
    • TestElasticsearchBackend.java

Motivation and Context

As I mentioned above, after 3 years of developing, publishing and maintaining the plugin "Elasticsearch Backend Listener", I believe it is time for it to be added to the main codebase. As a result of this, Elasticsearch would officially be supported by JMeter and would evolve nicely with it.

It is clearly a used and needed integration since, as it stands, there are over 2500 users installing the plugin (source).

How Has This Been Tested?

The plugin itself has been tested through and through for over 3 years. As for how it's been tested in the main codebase; I've changed all the packages names, added the necessary dependencies in the build.gradle.kts of the components workspace. I have applied the necessary coding styles, corrected and adjusted if necessary.

My testing environment consisted of the following:

  • locally built JMeter (5.3.1-SNAPSHOT + my code modifications)
  • ElasticSearch 7.9.0
  • Kibana 7.9.0

To test, I built JMeter using gw runGui. I then added a sample Thread Group as well as Debug samplers and a Backend Listener to which I setup the config to point to my local ElasticSearch engine.
I executed the test and verified that samples showed up.

Screenshots

Screen Shot 2020-09-01 at 3 14 00 PM
Screen Shot 2020-09-01 at 3 17 22 PM
Screen Shot 2020-09-01 at 3 23 35 PM

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • My code follows the code style of this project.
  • I have updated the documentation accordingly.

@anthonygauthier anthonygauthier force-pushed the master branch 2 times, most recently from 30202d8 to c79f1c8 Compare September 2, 2020 14:37
@anthonygauthier anthonygauthier marked this pull request as ready for review September 2, 2020 14:43
@anthonygauthier anthonygauthier changed the title Integrated ElasticSearch backend listener to main codebase Add: ElasticSearch backend listener Sep 4, 2020
@anthonygauthier
Copy link
Author

anthonygauthier commented Sep 13, 2020

Hey there @pmouawad , anything to change in your opinion?

@pmouawad
Copy link
Contributor

Hello @delirius325 ,
Thanks a lot for your contribution.
Can you open a discussion on dev mailing list for this PR ?

Thanks
Regards

@vlsi
Copy link
Collaborator

vlsi commented Sep 17, 2020

Just in case, here's dependency impact.
+5MiB is not that very expensive, however, it would be great if there was a brief explanation of why the dependency is needed and why existing classes are not enough.

For instance, we already have jackson-*, so do we really need gson?
Do we need guava?
Do we need AWS clients?

    55421535 => 60544843 bytes (+5123308 bytes)
    98 => 108 files (+10)
  
  + 1013903 aws-java-sdk-core-1.11.851.jar
  +  272295 aws-java-sdk-secretsmanager-1.11.851.jar
  +   12691 aws-signing-request-interceptor-0.0.22.jar
  +   64552 elasticsearch-rest-client-7.9.0.jar
  +  240255 gson-2.8.6.jar
  + 2256213 guava-18.0.jar
  +  565410 ion-java-1.0.2.jar
  +   48468 jackson-dataformat-cbor-2.6.7.jar
  +   27590 jmespath-java-1.11.851.jar
  +  621931 joda-time-2.8.1.jar

@anthonygauthier
Copy link
Author

@vlsi Thanks for the feedback! I've added comments concerning AWS and I've switched from Gson to Jackson, therefore avoiding the need to add a new dependency.

Updated realtime-results.xml doc

Removed ES related versions from gradle.propertiesÉ

Removed trailing lines in gradle.properties

Added license at the beginning of all new files

Removed org.json:json and usgin Gson instead

Correctly using Gson in getElasticSearchVersion()

Applied code style

Added local MIT license for aws-signing-request-interceptor

Updated dependencies, switched from gson to jackson

Applied styles
@pmouawad
Copy link
Contributor

pmouawad commented Oct 7, 2020

Hello @delirius325 ,
The drawbacks I see currently are:

  • For signing on AWS ElasticSearch, It introduces a dependency on aws-java-sdk and google guava which might not be acceptable. This should be available as a pluggable thing instead
  • We need to maintain in core a dependency on a 3rd party (Elastic Search) that has a faster pace in terms of releases
  • is there a reason for not keeping the plugin as is and contributing it to JMeter core ?

The addition looks really useful, but I am wondering if it's not better that this one remains as a plugin, as its pace of release can be faster than JMeter one.

What's your thoughts ?
Regards

@anthonygauthier
Copy link
Author

Hey @pmouawad ,

You bring up some good points; including the plugin's JAR probably would be the more logical and flexible approach to this. Otherwise, I agree that I would have to cut back on some of the features to remove certain dependencies (i.e. for AWS). Honestly, it's a good thing you brought this up, I hadn't even thought about doing it like that.

I'll look into making that happen instead of having an "official" implementation.

Thanks for the feedback!

@pmouawad
Copy link
Contributor

Thanks for your answer.
I'll close this PR for now.
Thanks

@pmouawad pmouawad closed this Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants