You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2022/09/05 14:57:14 UTC
[GitHub] [skywalking-java] pg-yang opened a new pull request, #303: Add tag db.type for encache,guava-cache plugin
pg-yang opened a new pull request, #303:
URL: https://github.com/apache/skywalking-java/pull/303
<!--
β οΈ Please make sure to read this template first, pull requests that don't accord with this template
maybe closed without notice.
Texts surrounded by `<` and `>` are meant to be replaced by you, e.g. <framework name>, <issue number>.
Put an `x` in the `[ ]` to mark the item as CHECKED. `[x]`
-->
<!-- ==== π Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist π ====
### Fix <bug description or the bug issue number or bug issue link>
- [ ] Add a unit test to verify that the fix works.
- [ ] Explain briefly why the bug exists and how to fix it.
==== π Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist π ==== -->
<!-- ==== π Remove this line WHEN AND ONLY WHEN you're adding a new plugin, follow the checklist π ====
### Add an agent plugin to support <framework name>
- [ ] Add a test case for the new plugin, refer to [the doc](https://github.com/apache/skywalking-java/blob/main/docs/en/setup/service-agent/java-agent/Plugin-test.md)
- [ ] Add a component id in [the component-libraries.yml](https://github.com/apache/skywalking/blob/master/oap-server/server-starter/src/main/resources/component-libraries.yml)
- [ ] Add a logo in [the UI repo](https://github.com/apache/skywalking-rocketbot-ui/tree/master/src/views/components/topology/assets)
==== π Remove this line WHEN AND ONLY WHEN you're adding a new plugin, follow the checklist π ==== -->
<!-- ==== π Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist π ====
### Improve the performance of <class or module or ...>
- [ ] Add a benchmark for the improvement, refer to [the existing ones](https://github.com/apache/skywalking-java/blob/main/apm-commons/apm-datacarrier/src/test/java/org/apache/skywalking/apm/commons/datacarrier/LinkedArrayBenchmark.java)
- [ ] The benchmark result.
```text
<Paste the benchmark results here>
```
- [ ] Links/URLs to the theory proof or discussion articles/blogs. <links/URLs here>
==== π Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist π ==== -->
<!-- ==== π Remove this line WHEN AND ONLY WHEN you're adding a new feature, follow the checklist π ====
### <Feature description>
- [ ] If this is non-trivial feature, paste the links/URLs to the design doc.
- [ ] Update the documentation to include this new feature.
- [ ] Tests(including UT, IT, E2E) are added to verify the new feature.
- [ ] If it's UI related, attach the screenshots below.
==== π Remove this line WHEN AND ONLY WHEN you're adding a new feature, follow the checklist π ==== -->
- [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #<issue number>.
- [ ] Update the [`CHANGES` log](https://github.com/apache/skywalking-java/blob/main/CHANGES.md).
First , Add db .type for guava , encache which miss the tag
related issue https://github.com/apache/skywalking/issues/9556
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#discussion_r963027689
##########
apm-sniffer/optional-plugins/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheLockInterceptor.java:
##########
@@ -49,6 +49,7 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
if (element != null) {
Tags.DB_STATEMENT.set(span, element.toString());
}
+ Tags.DB_TYPE.set(span, "Ehcache");
Review Comment:
What do you mean knowing method? Redis get API always requires a key
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#discussion_r963043036
##########
apm-sniffer/optional-plugins/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheLockInterceptor.java:
##########
@@ -49,6 +49,7 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
if (element != null) {
Tags.DB_STATEMENT.set(span, element.toString());
}
+ Tags.DB_TYPE.set(span, "Ehcache");
Review Comment:
Yes, it is not easy. I don't mean it has to be done.
As you asked for something to do? I provide as many ideas as possible.
This could cause several PR, several months to do. This happened many times. SkyWalking kernel had been written 4 times
πThis is hoe oss evaluation. We try to do long term plan step by step.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#issuecomment-1239224947
Make sense to me, please go ahead. We need to find a place to doc these tags and meanings in Java agent and backend.
I will back to this thread later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #303: Update guava-cache,jedis,memcached,ehcache plugins to adopt uniform tags
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#issuecomment-1242735181
> I'm disheartened to write doc . Thanks , You are patient
I would like to help with docs writing if you have any trouble.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on a diff in pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on code in PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#discussion_r963042471
##########
apm-sniffer/optional-plugins/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheLockInterceptor.java:
##########
@@ -49,6 +49,7 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
if (element != null) {
Tags.DB_STATEMENT.set(span, element.toString());
}
+ Tags.DB_TYPE.set(span, "Ehcache");
Review Comment:
I'm very understand that you want more metric . But I don't agree with creating fat-plugin
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#issuecomment-1237755485
> How about we provide mechanism for mapping command to op in OAP . we support two group (read/write) analysis .
> This mapping could be configured by user . User make that which command is write or read
> Or you perfect Plugin Tag read/write ?
It is better we don't do this. It is not a preference, this is a math theory. When you only cost 0.001 ms for every request to identify the type, you need to cost 0.001 ms * N request * M threads per JVM * Y JVM processes, which means you need at least 10-20 more CPUs at the OAP side to make this works.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#issuecomment-1237757931
So, We put the mapping configure to plugin side .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on a diff in pull request #303: Polish up tags for cache related plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on code in PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#discussion_r963011495
##########
apm-sniffer/optional-plugins/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheLockInterceptor.java:
##########
@@ -49,6 +49,7 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
if (element != null) {
Tags.DB_STATEMENT.set(span, element.toString());
}
+ Tags.DB_TYPE.set(span, "Ehcache");
Review Comment:
I found the comment about DB _TYPE
```java
/**
* DB_TYPE records database type, such as sql, redis, cassandra and so on.
*/
public static final StringTag DB_TYPE = new StringTag(3, "db.type");
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#issuecomment-1237750905
How about we provide mapping mechanism from command to op in OAP . we support two group (read/write) analysis .
This mapping could be configured by user . User make that which command is write or read
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#issuecomment-1237980128
`HelperInitiator` will be put to a new module named `virtual-cache-helper` , this module is submodule of `apm-sdk-plugin` , and it's responsible for parsing the configuration , providing uniform convert function
All cache plugin will depends this plugin .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#issuecomment-1240112282
Could you send a pull request to here https://skywalking.apache.org/docs/skywalking-java/latest/en/setup/service-agent/java-agent/java-plugin-development-guide/#advanced-apis (after the whole `Advanced APIs` section)?
We should add reserved tags, the new added tags and database relative tags should be defined here.
Database relative tags could refer from codes and the [backend doc](https://skywalking.apache.org/docs/main/latest/en/setup/service-agent/virtual-database/)
A new backend doc for `virtual-cache` should be added later
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on a diff in pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on code in PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#discussion_r963038786
##########
apm-sniffer/optional-plugins/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheLockInterceptor.java:
##########
@@ -49,6 +49,7 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
if (element != null) {
Tags.DB_STATEMENT.set(span, element.toString());
}
+ Tags.DB_TYPE.set(span, "Ehcache");
Review Comment:
We could provide a mapping mechanism .
The plugin only send statement to OAP , and OAP provide a mechanism to parse the statement to op/key
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#issuecomment-1238017178
Every cache plugin should have own config for Write/Read mapping , even if their comma is different . This module only math String command to operation .
If we don't want adding dependency to cache plugin , we could use `maven shade plugin ` for `merging ` common class to every cache plugin .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on a diff in pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on code in PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#discussion_r962999591
##########
apm-sniffer/optional-plugins/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheLockInterceptor.java:
##########
@@ -49,6 +49,7 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
if (element != null) {
Tags.DB_STATEMENT.set(span, element.toString());
}
+ Tags.DB_TYPE.set(span, "Ehcache");
Review Comment:
Must modify all cache plugin , such as jedis .
And `DB_STATEMENT` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on a diff in pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on code in PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#discussion_r963038786
##########
apm-sniffer/optional-plugins/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheLockInterceptor.java:
##########
@@ -49,6 +49,7 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
if (element != null) {
Tags.DB_STATEMENT.set(span, element.toString());
}
+ Tags.DB_TYPE.set(span, "Ehcache");
Review Comment:
We could provide a mapping mechanism in OAP
The plugin only send statement to OAP , and OAP provide could parse the statement to op/key and so on
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#issuecomment-1240252459
Let's finish `virtual-cache` first , then coding , we could agree through the `doc`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on a diff in pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on code in PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#discussion_r963025832
##########
apm-sniffer/optional-plugins/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheLockInterceptor.java:
##########
@@ -49,6 +49,7 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
if (element != null) {
Tags.DB_STATEMENT.set(span, element.toString());
}
+ Tags.DB_TYPE.set(span, "Ehcache");
Review Comment:
We could modify these tag , since they haven't be used.
We don't have clear way to extract cache.key , cache.op .
We can't guarantee developers must write code to provide correct `op ` ,`key ` at least .
in order to get correct `op ` ,`key `,Plugins must know java method which is involved by user , it's very difficult ,and ugly
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on a diff in pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on code in PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#discussion_r963044772
##########
apm-sniffer/optional-plugins/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheLockInterceptor.java:
##########
@@ -49,6 +49,7 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
if (element != null) {
Tags.DB_STATEMENT.set(span, element.toString());
}
+ Tags.DB_TYPE.set(span, "Ehcache");
Review Comment:
> We could provide a mapping mechanism in OAP The plugin only send statement to OAP , and OAP could parse the statement to op/key or other flag . So , user could know how it work clearly
How about this . I like
BTW , fat-plugin shoudn't exist
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#issuecomment-1238007705
Why? Their command should be different, I think we could have their own? It is better we don't set plugin dependencies. This would be more friendly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#issuecomment-1238002364
> Why we need to share? I think the command is tech stack relative. Do you want to redis clients sharing?
jedis , guava-cache, memercached , all cache plugin need to depend it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on pull request #303: Update guava-cache,jedis,memcached,ehcache plugins to adopt uniform tags
Posted by GitBox <gi...@apache.org>.
pg-yang commented on PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#issuecomment-1242734715
I'm disheartened to write doc . Thanks , You are patient
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on a diff in pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on code in PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#discussion_r963037702
##########
apm-sniffer/optional-plugins/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheLockInterceptor.java:
##########
@@ -49,6 +49,7 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
if (element != null) {
Tags.DB_STATEMENT.set(span, element.toString());
}
+ Tags.DB_TYPE.set(span, "Ehcache");
Review Comment:
-> cache.key
Some command don't have key , or the key can't be serialized
-> cache.op
For example , simulate java stack that call redis server , like this
```
get -> exe
put -> exe
```
We intercept exe method usually , in order to get op . we need know the method which is invoked by user (get /put ) .
Of course , We also could detect op from method argument , But sometimes the method don't have appropriate argument .
We also need a relationship for mapping something to op οΌ I think it need too many workοΌ and it very depends on developer 's idea or perception .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on a diff in pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on code in PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#discussion_r963038786
##########
apm-sniffer/optional-plugins/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheLockInterceptor.java:
##########
@@ -49,6 +49,7 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
if (element != null) {
Tags.DB_STATEMENT.set(span, element.toString());
}
+ Tags.DB_TYPE.set(span, "Ehcache");
Review Comment:
We could provide a mapping mechanism in OAP
The plugin only send statement to OAP , and OAP could parse the statement to op/key and so on
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#discussion_r963017607
##########
apm-sniffer/optional-plugins/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheLockInterceptor.java:
##########
@@ -49,6 +49,7 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
if (element != null) {
Tags.DB_STATEMENT.set(span, element.toString());
}
+ Tags.DB_TYPE.set(span, "Ehcache");
Review Comment:
I think we didn't analyze spans from those plugins, so, changing new tags for analyzing is not an issue. I noticed plugins like Jedis using database `statement` and `type`.
Also, how about we add a new `cache.key`(key for reading and writing) and `cache.op`(read/write/general) tags for those plugins too?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#issuecomment-1239222414
Update ,define following tag for all cache plugin
1. `cache.type` , represent which cache system is used
2. `cache.op` , represent the command for write or read
3. `cache.cmd` represent command which send to cache system
4. `cache.key` represent key which send to cache system , nullable
Provide following config for all cache plugin (Java filed format)
1. `Set<String> OPERATION_MAPPING_WRITE` , which command should be mapped to write
2. `Set<String> OPERATION_MAPPING_READ` ,which command should be mapped to read
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#issuecomment-1237642217
redis commands : https://github.com/redis/jedis/blob/master/src/main/java/redis/clients/jedis/Protocol.java#L218
memcached : https://github.com/memcached/memcached/wiki/Commands
redis is the most complex , I'll try it first .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on a diff in pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on code in PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#discussion_r963051879
##########
apm-sniffer/optional-plugins/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheLockInterceptor.java:
##########
@@ -49,6 +49,7 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
if (element != null) {
Tags.DB_STATEMENT.set(span, element.toString());
}
+ Tags.DB_TYPE.set(span, "Ehcache");
Review Comment:
> > > We could provide a mapping mechanism in OAP The plugin only send statement to OAP , and OAP could parse the statement to op/key or other flag . So , user could know how it work clearly
> >
> >
> > How about this . I like
> > BTW , fat-plugin shoudn't exist
>
> If you searched closed issues, you will find a SQL parser PoC, powered by Apache ShardingSphere. It failed due to perf impact, can't analyze SQL fast enough to get table read/write metrics.
>
> I don't think we could win this for cache. SQL is much better formated than No SQL or cache statements.
Yes , it maybe slow . Let me think about this problem.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#issuecomment-1237545597
Let's start from listed all relative plugins, which are cache plugins.
Then we could check tags usage and adjust them one by one.
- Make cache tags to replace database tags
- Add tags for keys
- Define op tags(read/write/enqueue/etc.) and implement them in plugins.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#issuecomment-1237933817
I have done a demo for plugin . I add a module named `virtual-cache-helper`.
Cache plugin must call `HelperInitiator#register` , this method will return a `Function` that convert command to op
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#issuecomment-1237970681
Where does this class place? I think this is plugin specific only?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#issuecomment-1238285735
> https://github.com/pg-yang/skywalking-java/blob/feature/virtual-cache-v2/apm-sniffer/apm-sdk-plugin/virtual-cache-helper/src/main/java/org/apache/skywalking/apm/plugin/vcache/helper/HelperInitiator.java
I didn't read this from my cellphone at this night. I just checked I think you made things too complex.
As a mapping, I think you just need a map like your [previous version](https://github.com/pg-yang/skywalking-java/blob/feature/virtual-cache-v2/apm-sniffer/apm-sdk-plugin/jedis-plugins/jedis-4.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/jedis/v4/OperationParser.java). Simple, fast and clear
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on a diff in pull request #303: Polish up tags for cache related plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on code in PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#discussion_r963011495
##########
apm-sniffer/optional-plugins/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheLockInterceptor.java:
##########
@@ -49,6 +49,7 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
if (element != null) {
Tags.DB_STATEMENT.set(span, element.toString());
}
+ Tags.DB_TYPE.set(span, "Ehcache");
Review Comment:
I found the comment about DB _TYPE
```
/**
* DB_TYPE records database type, such as sql, redis, cassandra and so on.
*/
public static final StringTag DB_TYPE = new StringTag(3, "db.type");
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#discussion_r963046636
##########
apm-sniffer/optional-plugins/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheLockInterceptor.java:
##########
@@ -49,6 +49,7 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
if (element != null) {
Tags.DB_STATEMENT.set(span, element.toString());
}
+ Tags.DB_TYPE.set(span, "Ehcache");
Review Comment:
> I understand that you want more metric . But I don't agree with creating fat-plugin
I don't want more metrics. SkyWalking costs a lots of energe to reduce resource costs.
My point is simple, if we cost timr to build a thing, we build a useful tool.
SkyWalking is not chasing a function on the menu, website or in the news. We try to do a right thing, which is a good APM tool to help ppl addressing perf issues.
OSS community is not like a daily work, we are not urgent.
Many details could be discussed during the process.
SkyWalking existing features are all being discussed many times, they are not done by an easy way for anyone.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#discussion_r963048668
##########
apm-sniffer/optional-plugins/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheLockInterceptor.java:
##########
@@ -49,6 +49,7 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
if (element != null) {
Tags.DB_STATEMENT.set(span, element.toString());
}
+ Tags.DB_TYPE.set(span, "Ehcache");
Review Comment:
> > We could provide a mapping mechanism in OAP The plugin only send statement to OAP , and OAP could parse the statement to op/key or other flag . So , user could know how it work clearly
>
>
>
> How about this . I like
>
> BTW , fat-plugin shoudn't exist
If you searched closed issues, you will find a SQL parser PoC, powered by Apache ShardingSphere. It failed due to perf impact, can't analyze SQL fast enough to get table read/write metrics.
I don't think we could win this for cache. SQL is much better formated than No SQL or cache statements.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on a diff in pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on code in PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#discussion_r963044772
##########
apm-sniffer/optional-plugins/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheLockInterceptor.java:
##########
@@ -49,6 +49,7 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
if (element != null) {
Tags.DB_STATEMENT.set(span, element.toString());
}
+ Tags.DB_TYPE.set(span, "Ehcache");
Review Comment:
> We could provide a mapping mechanism in OAP The plugin only send statement to OAP , and OAP could parse the statement to op/key or other flag . So , user could know how it work clearly
How about this . I like
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on a diff in pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on code in PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#discussion_r963038786
##########
apm-sniffer/optional-plugins/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheLockInterceptor.java:
##########
@@ -49,6 +49,7 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
if (element != null) {
Tags.DB_STATEMENT.set(span, element.toString());
}
+ Tags.DB_TYPE.set(span, "Ehcache");
Review Comment:
We could provide a mapping mechanism in OAP
The plugin only send statement to OAP , and OAP could parse the statement to op/key or other flag .
So , user could know how it work clearly
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on a diff in pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on code in PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#discussion_r963048844
##########
apm-sniffer/optional-plugins/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheLockInterceptor.java:
##########
@@ -49,6 +49,7 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
if (element != null) {
Tags.DB_STATEMENT.set(span, element.toString());
}
+ Tags.DB_TYPE.set(span, "Ehcache");
Review Comment:
I see , I don't work for a company .HaHa .
Le's talk about providing mapping mechanism in OAP for op/key
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#issuecomment-1237698803
We could provide a tag named `cache.command` , user can config which command should be analyzed in OAP .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #303: Update guava-cache,jedis,memcached,ehcache plugins to adapt uniform tag
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#issuecomment-1242631028
I will review the change, I think this PR doesn't have side effects. Users could use it for previous OAP, as we don't analysis anything before.
We could add v8.13.0 Java agent and required tags in the OAP doc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#issuecomment-1237744329
> We could provide a tag named `cache.command` , user can config which command should be analyzed in OAP . Or mapping command to op(write/read/etc) , It's simple string match , such as
>
> ```
> Write:
> - GET
> - HGET
> Read:
> - SET
> - SETNX
> - HSET
> ```
>
> Actually , business application just use a few command in cache system
Yes, we just need READ or WRITE tag if we could identify them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#issuecomment-1240143335
OK.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng merged pull request #303: Update guava-cache,jedis,memcached,ehcache plugins to adopt uniform tags
Posted by GitBox <gi...@apache.org>.
wu-sheng merged PR #303:
URL: https://github.com/apache/skywalking-java/pull/303
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#discussion_r963153926
##########
apm-sniffer/optional-plugins/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheLockInterceptor.java:
##########
@@ -49,6 +49,7 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
if (element != null) {
Tags.DB_STATEMENT.set(span, element.toString());
}
+ Tags.DB_TYPE.set(span, "Ehcache");
Review Comment:
At least in SkyWalking, we don't push contributors. You are already self driven, so we can have you as volunteers to build this. It is not fair we asked a volunteer to promise something must be done.
We have plans to release every 2 or 3 months, you could set a plan to finish this in one or two milestones, which means you could have half a year buffer.
So, don't hurry, and don't worry. And more ,Git is a time machine, we could revert halfly done things to keep release work, and revert the revert back to continue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on a diff in pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on code in PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#discussion_r962999591
##########
apm-sniffer/optional-plugins/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheLockInterceptor.java:
##########
@@ -49,6 +49,7 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
if (element != null) {
Tags.DB_STATEMENT.set(span, element.toString());
}
+ Tags.DB_TYPE.set(span, "Ehcache");
Review Comment:
Must modify all cache plugin , such jedis .
And `DB_STATEMENT` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on a diff in pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on code in PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#discussion_r963042471
##########
apm-sniffer/optional-plugins/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheLockInterceptor.java:
##########
@@ -49,6 +49,7 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
if (element != null) {
Tags.DB_STATEMENT.set(span, element.toString());
}
+ Tags.DB_TYPE.set(span, "Ehcache");
Review Comment:
I understand that you want more metric . But I don't agree with creating fat-plugin
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on a diff in pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on code in PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#discussion_r963051879
##########
apm-sniffer/optional-plugins/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheLockInterceptor.java:
##########
@@ -49,6 +49,7 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
if (element != null) {
Tags.DB_STATEMENT.set(span, element.toString());
}
+ Tags.DB_TYPE.set(span, "Ehcache");
Review Comment:
> > > We could provide a mapping mechanism in OAP The plugin only send statement to OAP , and OAP could parse the statement to op/key or other flag . So , user could know how it work clearly
> >
> >
> > How about this . I like
> > BTW , fat-plugin shoudn't exist
>
> If you searched closed issues, you will find a SQL parser PoC, powered by Apache ShardingSphere. It failed due to perf impact, can't analyze SQL fast enough to get table read/write metrics.
>
> I don't think we could win this for cache. SQL is much better formated than No SQL or cache statements.
Yes , it maybe slow . Let me think about this this conversation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on a diff in pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on code in PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#discussion_r963051879
##########
apm-sniffer/optional-plugins/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheLockInterceptor.java:
##########
@@ -49,6 +49,7 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
if (element != null) {
Tags.DB_STATEMENT.set(span, element.toString());
}
+ Tags.DB_TYPE.set(span, "Ehcache");
Review Comment:
> > > We could provide a mapping mechanism in OAP The plugin only send statement to OAP , and OAP could parse the statement to op/key or other flag . So , user could know how it work clearly
> >
> >
> > How about this . I like
> > BTW , fat-plugin shoudn't exist
>
> If you searched closed issues, you will find a SQL parser PoC, powered by Apache ShardingSphere. It failed due to perf impact, can't analyze SQL fast enough to get table read/write metrics.
>
> I don't think we could win this for cache. SQL is much better formated than No SQL or cache statements.
Yes , it maybe slow . Let me think about this conversation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#issuecomment-1237999459
Why we need to share? I think the command is tech stack relative.
Do you want to redis clients sharing?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#issuecomment-1238027288
Every plugin commands are different, I think copying the codes into various plugins under different package names is fine.
Jedis has nothing to share with Ehcache commands, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #303: Update guava-cache,jedis,memcached,ehcache plugins to adapt uniform tag
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#discussion_r967566375
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/tag/Tags.java:
##########
@@ -107,6 +107,28 @@ public static final class HTTP {
}
public static final StringTag LOGIC_ENDPOINT = new StringTag(12, "x-le");
+ /**
+ * CACHE_TYPE records cache type, such as jedis
+ */
+ public static final StringTag CACHE_TYPE = new StringTag(15, "cache.type");
+
+ /**
+ * CACHE_OP represent a command is used for "write" or "read"
+ * It's better that adding this tag to span , so OAP would analysis write/read metric accurately
+ * Reference org.apache.skywalking.apm.plugin.jedis.v4.AbstractConnectionInterceptor#parseOperation
+ * BTW "op" means Operation
+ */
+ public static final StringTag CACHE_OP = new StringTag(16, "cache.op");
+
+ /**
+ * CACHE_TYPE records the cache command
+ */
+ public static final StringTag CACHE_CMD = new StringTag(17, "cache.cmd");
+
+ /**
+ * CACHE_TYPE records the cache key
+ */
+ public static final StringTag CACHE_KEY = new StringTag(18, "cache.key");
Review Comment:
@apache/skywalking-committers Please notice, these new tags would be significant for virtual cache analysis, which would be added in 9.3.0.
If you want your cache framework relative plugins would work like these Java plugin, please follow the tag format protocol.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on pull request #303: Update guava-cache,jedis,memcached,ehcache plugins to adapt uniform tag
Posted by GitBox <gi...@apache.org>.
pg-yang commented on PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#issuecomment-1242627143
I think this PR would not be merged util the work in OAP finished , it's better wait I finish the test of OAP and plugin .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#discussion_r962995733
##########
apm-sniffer/optional-plugins/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheLockInterceptor.java:
##########
@@ -49,6 +49,7 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
if (element != null) {
Tags.DB_STATEMENT.set(span, element.toString());
}
+ Tags.DB_TYPE.set(span, "Ehcache");
Review Comment:
How about we add a `Tags.CACHE_TYPE`? As we have agreed that, database and cache are different.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#issuecomment-1237740633
To easy the configurations, I think we only need read/write and default(cmd in your configuration?). Then the worst case when we missed something, is that, we don't treat it as a read/write command.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#discussion_r963043036
##########
apm-sniffer/optional-plugins/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheLockInterceptor.java:
##########
@@ -49,6 +49,7 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
if (element != null) {
Tags.DB_STATEMENT.set(span, element.toString());
}
+ Tags.DB_TYPE.set(span, "Ehcache");
Review Comment:
Yes, it is not easy. I don't mean it has to be done.
As you asked for something to do? I provide as many ideas as possible.
This could cause several PR, several months to do. This happened many times. SkyWalking kernel had been written 4 times
πThis is how oss evaluation happens. We try to do long term plan step by step.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on a diff in pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on code in PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#discussion_r963042471
##########
apm-sniffer/optional-plugins/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheLockInterceptor.java:
##########
@@ -49,6 +49,7 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
if (element != null) {
Tags.DB_STATEMENT.set(span, element.toString());
}
+ Tags.DB_TYPE.set(span, "Ehcache");
Review Comment:
I'm very understand that you want more metric . But I don't agree create fat-plugin
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#issuecomment-1238289776
No problem , Just need a config .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on a diff in pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on code in PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#discussion_r963038786
##########
apm-sniffer/optional-plugins/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheLockInterceptor.java:
##########
@@ -49,6 +49,7 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
if (element != null) {
Tags.DB_STATEMENT.set(span, element.toString());
}
+ Tags.DB_TYPE.set(span, "Ehcache");
Review Comment:
We could provide a mapping mechanism in OAP
The plugin only send statement to OAP , and OAP could parse the statement to op/key or other flag
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on a diff in pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on code in PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#discussion_r963063382
##########
apm-sniffer/optional-plugins/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheLockInterceptor.java:
##########
@@ -49,6 +49,7 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
if (element != null) {
Tags.DB_STATEMENT.set(span, element.toString());
}
+ Tags.DB_TYPE.set(span, "Ehcache");
Review Comment:
At first I wanted to try something , but I always want to finish it as possible as soon , even if no one is urging me . Sorry for my mood .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#issuecomment-1237657209
Look like this
```java
public class OperatorParser {
private static final Map<Command, String> MAPPING = new HashMap<>();
static {
MAPPING.put(Command.GET, "READ");
MAPPING.put(Command.HGET, "READ");
MAPPING.put(Command.MGET, "READ");
MAPPING.put(Command.ZCARD, "READ");
MAPPING.put(Command.ZCOUNT, "READ");
// ....
// ....
}
public static String parse(Command command) {
return Optional.ofNullable(command).map(MAPPING::get).orElse("Other");
}
}
```
Do you have better way ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] pg-yang commented on pull request #303: Add tag db.type for encache,guava-cache plugin
Posted by GitBox <gi...@apache.org>.
pg-yang commented on PR #303:
URL: https://github.com/apache/skywalking-java/pull/303#issuecomment-1237701568
https://github.com/pg-yang/skywalking-java/blob/feature/virtual-cache-v2/apm-sniffer/apm-sdk-plugin/jedis-plugins/jedis-4.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/jedis/v4/OperationParser.java
It's my code for converting command to `op `.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org