Provide plugin for Solr-7.x(client) by dmsolr · Pull Request #2730 · apache/skywalking

dmsolr · 2019-05-22T06:18:56Z

Please answer these questions before submitting pull request

Why submit this pull request?
Bug fix
New feature provided
Improve performance
Related issues
[Agent] Provide plugin for Solr-7.x #2501

wu-sheng · 2019-05-22T06:20:26Z

Waiting for the integration test pull request.

coveralls · 2019-05-22T06:50:11Z

Coverage increased (+0.2%) to 17.185% when pulling f24d46c on dmsolr:solrj-plugin into 3217ff1 on apache:master.

wu-sheng · 2019-05-22T13:16:46Z

+                    .setLayer(SpanLayer.DB);
+            span.tag(SolrjTags.TAG_QT, params.get(CommonParams.QT, "/select"));
+        }
+


Above codes include a lot of instanceof, which would be an impact for performance, especially in JRE 6-8. If possible, please consider a better way to do this.

kezhenxu94

inline

…nto solrj-plugin

SkyWalkingRobot · 2019-05-23T12:19:34Z

Here is the test report and validate logs

wu-sheng · 2019-05-23T12:26:03Z

@dmsolr The test fails, please make sure your local test passed.

dmsolr · 2019-05-23T13:13:53Z

I think that is why I committed the expected data of test cases that dosen't take effect.

I modified the tags' sequence.

wu-sheng · 2019-05-23T13:27:37Z

I think that is why I committed the expected data of test cases that dosen't take effect.
I modified the tags' sequence.

The robot will use the latest commit of this and SkyAPMTest/agent-auto-integration-testcases#72 . What do you mean dosen't take effect.?

dmsolr · 2019-05-29T10:32:38Z

I talk with @wu-sheng about how to deal status of span in the afterMethod and handleMethodException.

Skywalking care about performance very much.
Interceptor will miss to trace customized Requests of SolrJ users.

So I decide to trace all of path.

Test Report - 2019-05-29 18:14

tests : 12 passed. 0 failed

branch name : solrj-plugin

commit id : d3ce6413e8719807633dd4c62d214069c75a799f

cases branch: master

cases commit id: b81bf75c06294957470107264ce80fc44cd45924

Cases List

Case description Status Cases

solrj 12 passed. 0 failed click me

IanCao · 2019-05-31T02:35:28Z

inline

dmsolr · 2019-05-31T03:19:43Z

Hi @IanCao, I had resolved the checking conditions must be in pairs.
I will create the span anytime, so I think the re-checking conditions is ok in afterMethod().
How about you?

SkyWalkingRobot · 2019-05-31T03:32:26Z

Here is the test report and validate logs

IanCao · 2019-05-31T03:39:06Z

Hi @IanCao, I had resolved the checking conditions must be in pairs.
I will create the span anytime, so I think the re-checking conditions is ok in afterMethod().
How about you?

yes.in my opinion , it's ok
and test passed

IanCao · 2019-05-31T03:40:39Z

lgtm @wu-sheng

wu-sheng · 2019-05-31T05:33:52Z

+        <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
+        <java.version>1.6</java.version>
+        <maven.compiler.source>${java.version}</maven.compiler.source>
+        <maven.compiler.target>${java.version}</maven.compiler.target>


You don't need these three, L35-L37.

wu-sheng

I still need a little explanation about the tags, because from my understanding, SolrJ is the first plugin to set up its own tags. To be clear, new tags are not bad things, I just want to make sure we make things good, and keep SkyWalking in a good performance as always.

Could you brief about what these tags for? Clearly, it is not for analysis, because no backend codes will know that. Such as,

Are these tags like SQL statement?
Are these tags including parameters of each request?
Do we have other choices to format these, such as format it as a JSON into DB_STATEMENT tag? The reason I asked this is because it will benefit from existing DB metrics and slow statement analysis.

You have already defined the SolrJ as DB layer, so for me, I think following the existing DB plugin tradition makes more sense.

Look forward to having your reply.

wu-sheng

Also, as a DB layer, you don't set DB_INSTANCE and DB_TYPE tags. These are important tags, with DB_STATEMENT in above comment.

dmsolr · 2019-05-31T13:14:02Z

Hi @wu-sheng ,

common tags:

status : status of response.
qtime : solr

query tags

qt: Query Type - which Request Handler should handle the request.
numFound : the numer of SolrDocuments returned from a search.
db.statment: If it enable, span will trace the query parameters of query. (Default disable)

insert tags(/update/ADD)

docsSize: the number of committed documents.
commitWithin: the time horizon by which the document should be committed

delete tag(/update/DELETE_X)

delete.by : deletes a list of documents by doc_id. (or deletes documents base on query)

commit tags (/update/COMMIT)

softCommit : Performs an explicit commit, causing pending documents to be committed for indexing.

optimize tags (/update/OPTIMIZE)

maxOptimizeSegs : optimizes down to at most this number of segments

On commit/optimize request，maybe need to trace waitFLush and waitSearch. They explain whether QTime is available.

About DB_INSTANCE, for solr, to get this is expensive and useless.
About db.statement, it will be encode by URLEncoder. I have not idea whether to decode it.

wu-sheng · 2019-05-31T15:34:05Z

@dmsolr I just read your reply again, here is my reply

status : status of response.

Why this status doesn't match span status? Which values of these status?

qtime : solr

Why doesn't this match the span duration? If this represents target(solr server) duration, we should have solr server trace in next step, right? Then we still don't need this.

docsSize
commitWithin
softCommit
delete.by
softCommit
maxOptimizeSegs
waitFLush
waitSearch

All these are either execution results or parameters. In my mind, all these should only be tagged when the user explicitly opens it. Add config item to Config#Plugin#SolrJ.
Because all these String tags cost a lot of network and storage.

For URLEncoder, it is a very simple encode OP, you could easily decode it. Just google it :) I think that perfectly match the statement.

And in your example, could you tell me, what does mycore mean? Look like your operation name include it.

Out of the above context, my other question is, what is purpose and benefit to provide Solr server plugin.
Thinking in these ways

We have no intention to improve solr server performance.
We analysis database performance based on client(Exist Span), read `MultiScopesSpanListener#L162). It will be confusing, both service metrics and database metrics dashboards showing with different names. Service dashboard shows agent config service_code, database dashboards show based on client(solrJ) peer.

The point is, I hope we could focus on APM, and don't get user confused.

dmsolr · 2019-06-03T05:51:36Z

@wu-sheng , I am sorry to reply so late.

Why this status doesn't match span status? Which values of these status?

It looks like http status. But it uses 0 instead of 2xx and uses 500 instead of less than 100.

Why doesn't this match the span duration? If this represents target(solr server) duration, we should have solr server trace in next step, right? Then we still don't need this.

Yes. It represents elapsed time on server side. It exclude http, de/en-serialized time-consuming.

And in your example, could you tell me, what does mycore mean? Look like your operation name include it.

That likes table name in SQL. It consist of shards and replicas. If the request sends to the specified sharding, the operation name will use sharding name( it names collectionName_shardingNum_replicaNum). However collection name will be contained. So it can be removed.

About docsSize, delete.by.

I think they are useful. delete.by likes deleting statement in SQL. I will move it to db.statement. docsSize represents the number of effected rows in the upserting.

commitWithin, softCommit, maxOptimizeSegs

They may not be useful for single request analysis, but they let us know what is done to the server and what state the server is in. But for caller, they are really useless. For me, a ops of solr, they are a bit important. So they are opened through config.

wu-sheng · 2019-06-03T06:18:19Z

+        if (result != null) {
+            NamedList<Object> header = (NamedList<Object>) result.get("responseHeader");
+            if (header != null) {
+                span.tag(SolrjTags.TAG_STATUS, String.valueOf(header.get("status")));


By following your explanation, this tag value should work with span#errorOccurred together.

wu-sheng · 2019-06-03T06:24:48Z

@dmsolr Most are good to me, just one change required.

dmsolr · 2019-06-03T12:00:01Z

Test Report - 2019-06-03 19:58

tests : 12 passed. 0 failed

branch name : solrj-plugin

commit id : d3ce6413e8719807633dd4c62d214069c75a799f

cases branch: master

cases commit id: 13ab44789f82a6bbe8769d363706e236dcde26be

Cases List

Case description Status Cases

solrj 12 passed. 0 failed click me

SkyWalkingRobot · 2019-06-04T04:08:13Z

Here is the test report and validate logs

wu-sheng

Add the config to document of setup

wu-sheng

LGTM, @ascrutae Recheck?

SkyWalkingRobot · 2019-06-17T09:07:43Z

Here is the test report and validate logs

daming added 3 commits May 22, 2019 11:24

new branch for solrj-plugin

de5b793

rollback and add module solrj-plugin

1031356

preparing to pr

0ce3017

wu-sheng requested review from IanCao and zhaoyuguang May 22, 2019 06:19

wu-sheng mentioned this pull request May 22, 2019

add solrj-scenario SkyAPMTest/agent-auto-integration-testcases#72

Merged

wu-sheng requested a review from ascrutae May 22, 2019 06:52

wayilau requested changes May 22, 2019

View reviewed changes

Comment thread ...j-7.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/solrj/SolrClientInterceptor.java Outdated

wu-sheng reviewed May 22, 2019

View reviewed changes

Comment thread ...j-7.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/solrj/SolrClientInterceptor.java Outdated

wu-sheng reviewed May 22, 2019

View reviewed changes

Comment thread apm-sniffer/apm-sdk-plugin/solrj-7.x-plugin/src/main/resources/skywalking-plugin.def Outdated

kezhenxu94 reviewed May 22, 2019

View reviewed changes

daming and others added 5 commits May 23, 2019 14:35

to resolve reviewers' suggestions

9151a87

Merge branch 'master' into solrj-plugin

109d478

remove unused code

10d923c

Merge branch 'solrj-plugin' of https://github.com/dmsolr/skywalking i…

c1beec4

…nto solrj-plugin

remove unused code

e6b4d32

wu-sheng removed the TBD To be decided later, need more discussion or input. label May 23, 2019

Merge branch 'master' into solrj-plugin

65d7221

wu-sheng reviewed May 31, 2019

View reviewed changes

remove unnecessary properites

57d76d3

wu-sheng reviewed May 31, 2019

View reviewed changes

wu-sheng reviewed Jun 3, 2019

View reviewed changes

Merge branch 'master' into solrj-plugin

549be85

remove unnecessary tags

a310a94

Merge branch 'master' into solrj-plugin

0da3a38

wu-sheng reviewed Jun 4, 2019

View reviewed changes

daming and others added 2 commits June 4, 2019 14:18

Add the config to document of setup

74b1fc7

Merge branch 'master' into solrj-plugin

f24d46c

wu-sheng approved these changes Jun 4, 2019

View reviewed changes

ascrutae approved these changes Jun 5, 2019

View reviewed changes

wu-sheng merged commit 6ae5174 into apache:master Jun 5, 2019

dmsolr mentioned this pull request Jun 17, 2019

Restore solrj-plugin #2888

Merged

3 tasks

dmsolr deleted the solrj-plugin branch July 16, 2019 03:50

Conversation

dmsolr commented May 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wu-sheng commented May 22, 2019

Uh oh!

coveralls commented May 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

wu-sheng May 22, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kezhenxu94 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SkyWalkingRobot commented May 23, 2019

Uh oh!

wu-sheng commented May 23, 2019

Uh oh!

dmsolr commented May 23, 2019

Uh oh!

wu-sheng commented May 23, 2019

Uh oh!

dmsolr commented May 29, 2019

Test Report - 2019-05-29 18:14

Cases List

Uh oh!

IanCao commented May 31, 2019

Uh oh!

dmsolr commented May 31, 2019

Uh oh!

SkyWalkingRobot commented May 31, 2019

Uh oh!

IanCao commented May 31, 2019

Uh oh!

IanCao commented May 31, 2019

Uh oh!

wu-sheng May 31, 2019

Choose a reason for hiding this comment

Uh oh!

wu-sheng left a comment

Choose a reason for hiding this comment

Uh oh!

wu-sheng left a comment

Choose a reason for hiding this comment

Uh oh!

dmsolr commented May 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wu-sheng commented May 31, 2019

Uh oh!

dmsolr commented Jun 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wu-sheng Jun 3, 2019

Choose a reason for hiding this comment

Uh oh!

dmsolr Jun 3, 2019

Choose a reason for hiding this comment

Uh oh!

wu-sheng commented Jun 3, 2019

Uh oh!

dmsolr commented Jun 3, 2019

Test Report - 2019-06-03 19:58

Cases List

Uh oh!

SkyWalkingRobot commented Jun 4, 2019

Uh oh!

wu-sheng left a comment

Choose a reason for hiding this comment

Uh oh!

wu-sheng left a comment

Choose a reason for hiding this comment

Uh oh!

dmsolr commented May 22, 2019 •

edited

Loading

coveralls commented May 22, 2019 •

edited

Loading

dmsolr commented May 31, 2019 •

edited

Loading

dmsolr commented Jun 3, 2019 •

edited

Loading