Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Elasticsearch Client bboss plugin #5596

Closed
wants to merge 25 commits into from

Conversation

yin-bp
Copy link
Contributor

@yin-bp yin-bp commented May 13, 2019

Title: Elasticsearch Client Plugin Contribution

Link: https://github.com/bbossgroups/pinpoint/tree/elasticsearch-plugin/plugins/elasticsearch-bboss
Target: Elasticsearch Rest Client BBoss
Supported Version: pinpoint 1.9.0
Description: This agent plugin is worked with elasticsearch rest client BBoss,BBoss is a best Java Highlevel Rest client for ElasticSearch.

ServiceTypes: [9201, "ElasticsearchBBoss"],[9202, "ElasticsearchBBossExecutor"]
Annotations: [971, "es.args"],[972, "es.url"],[973, "es.dsl"],[974, "es.action"],[975, "es.responseHandle"],[976, "es.version"]

Configurations: List of configuration keys and description the plugin adds.

profiler.elasticsearchbboss.enabled=true //enable or disable this plugin, default true.
profiler.elasticsearchbboss.recordResult=false //record or disable record method return value ,default false.
profiler.elasticsearchbboss.recordArgs=true //record or disable record method args,default true.
profiler.elasticsearchbboss.recordDsl=true //record or disable record dsl,default true.
profiler.elasticsearchbboss.recordESVersion=true //record or disable record ES Version infomation,default true.
profiler.elasticsearchbboss.recordResponseHandlerClass=false //record or disable record response handle class ,default false.
profiler.elasticsearchbboss.maxDslSize=50000 //set the max length dsl script,over this size dsl will be substring.

@yin-bp
Copy link
Contributor Author

yin-bp commented May 13, 2019

Hi,@RoySRose
A clean PR was added!

@codecov-io
Copy link

codecov-io commented May 13, 2019

Codecov Report

Merging #5596 into master will decrease coverage by 0.01%.
The diff coverage is 45.73%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #5596      +/-   ##
=========================================
- Coverage   40.51%   40.5%   -0.02%     
=========================================
  Files        2865    2878      +13     
  Lines       86459   86783     +324     
  Branches    11444   11490      +46     
=========================================
+ Hits        35031   35150     +119     
- Misses      48390   48581     +191     
- Partials     3038    3052      +14
Impacted Files Coverage Δ
...icsearchbboss/ElasticsearchCustomMethodFilter.java 100% <100%> (ø)
...sticsearchbboss/RestSeachExecutorMethodFilter.java 100% <100%> (ø)
.../elasticsearchbboss/ElasticsearchPluginConfig.java 100% <100%> (ø)
...boss/interceptor/ParallelWorkerRunInterceptor.java 100% <100%> (ø)
...plugin/elasticsearchbboss/ElasticsearchPlugin.java 23.8% <23.8%> (ø)
...tor/ElasticsearchExecutorOperationInterceptor.java 35.36% <35.36%> (ø)
...asticsearchOperationAsyncInitiatorInterceptor.java 45.45% <45.45%> (ø)
...interceptor/ElasticsearchOperationInterceptor.java 61.53% <61.53%> (ø)
...sticsearchbboss/ElasticsearchMetadataProvider.java 81.81% <81.81%> (ø)
...erceptor/ParallelWorkerConstructorInterceptor.java 83.33% <83.33%> (ø)
... and 32 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2ddee6...b2d9bbf. Read the comment docs.

@RoySRose
Copy link
Collaborator

@yin-bp

Great Job.

  1. I don't really see any problems in the logic for now. I'll also request reviews to my colleagues.
  2. Please change the name of service types under ElasticSearchConstants.
    ServiceTypes are shared through all Pinpoint Module(Web, Collector ...)
    words in service types to change from Elasticsearch to ElasticsearchBBoss would be enough
  3. About ElasticsearchExecutorIT Test. This fails for now.
    Integration Test with embedded Elasticsearch such as https://github.com/allegro/embedded-elasticsearch would be great. You can look at MongoDBIT_3_7_x_IT.java as an example. The test result of this IntegrationTest is applied to Homepage automatically. So I strongly recommend to get it correctly.
  4. Do you have any example app for me to test with? I remember there was some in the past, and I kind of modified it to test and getting errors. So if you have any updated example. Please let me know :)

@yin-bp
Copy link
Contributor Author

yin-bp commented May 14, 2019

@RoySRose :
Here is a bboss web demo base spring boot and elasticsearch 5.x,6.x:
https://github.com/bbossgroups/es_bboss_web

Here is a quickstart tutorial:
https://esdoc.bbossgroups.com/#/quickstart

All the Suggestions above have been adopted and the code has been submitted

@yin-bp
Copy link
Contributor Author

yin-bp commented May 15, 2019

@RoySRose
Integration tests fail by adopting embedded elasticsearch. See continuous-integration/travis-ci/pr detail,how to handle errors in integration tests

@RoySRose
Copy link
Collaborator

@yin-bp

Does Elasticsearch-bboss support jdk6?

RoySRose added a commit to RoySRose/pinpoint that referenced this pull request May 17, 2019
@RoySRose
Copy link
Collaborator

@yin-bp

Actually, there isn't any problem with the existing test code.
I've looked up to find the reason and embedded-elasticsearch doesn't support JDK6.
So I've made a new sub-module for Integration Test with JDK8 (agent-it8)

I've merged PR #5596 to the master.
You can see the changes by rebasing your branch with master.

I've modified //TODO just a sample part, just to stop generating test fails.

I think it will be wise to version 5.x of elasticsearch rather than 6.3.0.
I don't know why but. 6.3.0 take so long to come up compared to 5.x(If you can find a better one~ please do)

Let me know when you're done

@yin-bp
Copy link
Contributor Author

yin-bp commented May 17, 2019

@RoySRose
image
How can I rebasing my branch with pinpoint master?
When Use the method above image, Other commits will be include in my branch. And you will see these commits in this pull request. How to prevent this?

@RoySRose
Copy link
Collaborator

Hello, @yin-bp Please check here https://blog.algolia.com/master-git-rebase/ this kindly explains about the rebase I was talking about. I use few tools such as sourcetree to easily manage git operations.
https://blog.algolia.com/wp-content/uploads/2017/12/image1.png this image shows most simplified version of what I'm talking about.

@yin-bp
Copy link
Contributor Author

yin-bp commented May 17, 2019

@RoySRose

@yin-bp

Does Elasticsearch-bboss support jdk6?

Do not support jdk 6,must be jdk 7 and upper.
I just do a rebase and merge operation from pinpoint master to my elasticsearch-plugin branch,Is that ok?

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
6 out of 7 committers have signed the CLA.

✅ binDongKim
✅ koo-taejin
✅ RoySRose
✅ yin-bp
✅ jaehong-kim
✅ denzelsN
❌ minwoo-jung
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

None yet

9 participants