Update guava-cache,jedis,memcached,ehcache plugins to adopt uniform tags #303

pg-yang · 2022-09-05T14:57:12Z

If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #.
Update the CHANGES log.

First , Add db .type for guava , encache which miss the tag

wu-sheng · 2022-09-05T15:09:05Z

...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");


How about we add a Tags.CACHE_TYPE? As we have agreed that, database and cache are different.

Must modify all cache plugin , such as jedis .

And DB_STATEMENT ?

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");

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?

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

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.

I see , I don't work for a company .HaHa .
Le's talk about providing mapping mechanism in OAP for op/key

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.

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 .

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.

wu-sheng · 2022-09-06T00:30:58Z

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.

pg-yang · 2022-09-06T04:21:02Z

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 .

pg-yang · 2022-09-06T04:52:52Z

Look like this

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 ?

pg-yang · 2022-09-06T06:09:32Z

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

pg-yang · 2022-09-06T06:13:46Z

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 .

wu-sheng · 2022-09-06T07:04:59Z

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.

wu-sheng · 2022-09-06T07:09:17Z

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.

pg-yang · 2022-09-06T07:15:36Z

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 ?

wu-sheng · 2022-09-06T07:20:39Z

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 at the backend. 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.

pg-yang · 2022-09-06T07:23:15Z

So, We put the mapping configure to plugin side .

pg-yang · 2022-09-06T09:59:01Z

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
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

wu-sheng · 2022-09-06T10:33:19Z

Where does this class place? I think this is plugin specific only?

pg-yang · 2022-09-06T10:43:11Z

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 .

wu-sheng · 2022-09-06T11:02:52Z

Why we need to share? I think the command is tech stack relative.
Do you want to redis clients sharing?

pg-yang · 2022-09-06T11:05:53Z

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

wu-sheng · 2022-09-06T11:11:25Z

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.

pg-yang · 2022-09-06T11:20:13Z

Every cache plugin should have own config for Write/Read mapping , even if their command is different . This module only convert command to operation .
If we don't want adding dependency to cache plugin , we could use maven shade plugin for merging common class to each cache plugin , or writing duplicate code for these cache plugin

wu-sheng · 2022-09-06T11:30:50Z

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?

wu-sheng · 2022-09-06T15:09:16Z

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. Simple, fast and clear

pg-yang · 2022-09-06T15:12:36Z

No problem , Just need a config .

pg-yang · 2022-09-07T10:45:36Z

Update ,define following tag for all cache plugin

cache.type , represent which cache system is used
cache.op , represent the command is used for write or read
cache.cmd represent command which is send to cache system
cache.key represent key which is send to cache system , nullable

Provide following config for all cache plugin (Java filed format)

Set<String> OPERATION_MAPPING_WRITE , which command should be mapped to write
Set<String> OPERATION_MAPPING_READ ,which command should be mapped to read

wu-sheng · 2022-09-07T10:48:11Z

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.

wu-sheng · 2022-09-08T01:28:06Z

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

A new backend doc for virtual-cache should be added later

pg-yang · 2022-09-08T02:28:55Z

OK.

pg-yang · 2022-09-08T05:52:28Z

Let's finish virtual-cache first , then coding , we could agree through the doc.

wu-sheng · 2022-09-10T02:08:55Z

...ffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/tag/Tags.java

+    /**
+     * 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");


@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.

pg-yang · 2022-09-10T04:42:01Z

I think this PR would not be merged until the work in OAP finished , it's better wait I finish the test of OAP and plugin .

wu-sheng · 2022-09-10T05:02:23Z

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.

wu-sheng

LGTM

wu-sheng · 2022-09-10T13:58:49Z

I'm disheartened to write doc . Thanks , You are patient

I would like to help with docs writing if you have any trouble.

wu-sheng reviewed Sep 5, 2022

View reviewed changes

wu-sheng added enhancement New feature or request plugin labels Sep 5, 2022

wu-sheng added this to the 8.13.0 milestone Sep 5, 2022

pg-yang changed the title ~~Add tag db.type for encache,guava-cache plugin~~ Polish up tags for cache related plugin Sep 5, 2022

pg-yang changed the title ~~Polish up tags for cache related plugin~~ Add tag db.type for encache,guava-cache plugin Sep 5, 2022

pg-yang force-pushed the feature/virtual-cache branch 2 times, most recently from d7b6a74 to b0849d1 Compare September 9, 2022 18:26

pg-yang changed the title ~~Add tag db.type for encache,guava-cache plugin~~ Update guava-cache,jedis,memcached,ehcache plugins to adapt uniform tag @pg-yang Sep 9, 2022

pg-yang changed the title ~~Update guava-cache,jedis,memcached,ehcache plugins to adapt uniform tag @pg-yang~~ Update guava-cache,jedis,memcached,ehcache plugins to adapt uniform tag Sep 9, 2022

Update guava-cache,jedis,memcached,ehcache plugins to adapt uniform tag

7200ffa

pg-yang force-pushed the feature/virtual-cache branch from b0849d1 to 7200ffa Compare September 10, 2022 01:44

wu-sheng reviewed Sep 10, 2022

View reviewed changes

wu-sheng added 2 commits September 10, 2022 21:23

Update Java-Plugin-Development-Guide.md

9784c71

Merge branch 'main' into feature/virtual-cache

74bcc1b

wu-sheng changed the title ~~Update guava-cache,jedis,memcached,ehcache plugins to adapt uniform tag~~ Update guava-cache,jedis,memcached,ehcache plugins to adopt uniform tags Sep 10, 2022

wu-sheng approved these changes Sep 10, 2022

View reviewed changes

wu-sheng merged commit 6fce184 into apache:main Sep 10, 2022

pg-yang deleted the feature/virtual-cache branch September 10, 2022 14:03

pg-yang mentioned this pull request Sep 13, 2022

[Feature] Add metric for analyzing cache-related span like Virtual Database apache/skywalking#9556

Closed

7 tasks

Update guava-cache,jedis,memcached,ehcache plugins to adopt uniform tags #303

Update guava-cache,jedis,memcached,ehcache plugins to adopt uniform tags #303

Conversation

pg-yang commented Sep 5, 2022 • edited

Choose a reason for hiding this comment

pg-yang Sep 5, 2022 • edited

Choose a reason for hiding this comment

pg-yang Sep 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pg-yang Sep 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wu-sheng commented Sep 6, 2022

pg-yang commented Sep 6, 2022

pg-yang commented Sep 6, 2022

pg-yang commented Sep 6, 2022 • edited

pg-yang commented Sep 6, 2022

wu-sheng commented Sep 6, 2022

wu-sheng commented Sep 6, 2022

pg-yang commented Sep 6, 2022 • edited

wu-sheng commented Sep 6, 2022 • edited

pg-yang commented Sep 6, 2022

pg-yang commented Sep 6, 2022 • edited

wu-sheng commented Sep 6, 2022

pg-yang commented Sep 6, 2022

wu-sheng commented Sep 6, 2022

pg-yang commented Sep 6, 2022

wu-sheng commented Sep 6, 2022

pg-yang commented Sep 6, 2022 • edited

wu-sheng commented Sep 6, 2022

wu-sheng commented Sep 6, 2022

pg-yang commented Sep 6, 2022

pg-yang commented Sep 7, 2022 • edited

wu-sheng commented Sep 7, 2022

wu-sheng commented Sep 8, 2022

pg-yang commented Sep 8, 2022

pg-yang commented Sep 8, 2022

Choose a reason for hiding this comment

pg-yang commented Sep 10, 2022 • edited

wu-sheng commented Sep 10, 2022

wu-sheng left a comment

Choose a reason for hiding this comment

wu-sheng commented Sep 10, 2022

pg-yang commented Sep 5, 2022 •

edited

pg-yang Sep 5, 2022 •

edited

pg-yang Sep 5, 2022 •

edited

pg-yang Sep 5, 2022 •

edited

pg-yang commented Sep 6, 2022 •

edited

pg-yang commented Sep 6, 2022 •

edited

wu-sheng commented Sep 6, 2022 •

edited

pg-yang commented Sep 6, 2022 •

edited

pg-yang commented Sep 6, 2022 •

edited

pg-yang commented Sep 7, 2022 •

edited

pg-yang commented Sep 10, 2022 •

edited