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 2020/02/17 16:23:34 UTC
[GitHub] [skywalking] lsyf opened a new pull request #4379: Fix ehcache: missing interceptor of private constructor called by clone method
lsyf opened a new pull request #4379: Fix ehcache: missing interceptor of private constructor called by clone method
URL: https://github.com/apache/skywalking/pull/4379
Please answer these questions before submitting pull request
- Why submit this pull request?
- [x] Bug fix
- [ ] New feature provided
- [ ] Improve performance
___
### Bug fix
- Bug description.
When the instance of `Cache` is created by the `clone` method, all the codes `EnhancedInstance # getSkyWalkingDynamicField ()` will return null in the ehcache interceptor .
**This will cause a `NullPointerException`.**
Beacase the code `EnhancedInstance # setSkyWalkingDynamicField()` is only called in the constructor's interceptor. But the private constructor can not intercepted by the interceptor.
- How to fix?
Add interceptors to all methods that call private constructors.
Only the `clone` method calls 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] lsyf commented on a change in pull request #4379: Fix ehcache: missing interceptor of private constructor and setName method
Posted by GitBox <gi...@apache.org>.
lsyf commented on a change in pull request #4379: Fix ehcache: missing interceptor of private constructor and setName method
URL: https://github.com/apache/skywalking/pull/4379#discussion_r381051680
##########
File path: apm-sniffer/apm-sdk-plugin/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheConstructorInterceptor.java
##########
@@ -18,18 +18,28 @@
package org.apache.skywalking.apm.plugin.ehcache.v2;
+import net.sf.ehcache.Cache;
import net.sf.ehcache.config.CacheConfiguration;
import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceConstructorInterceptor;
public class EhcacheConstructorInterceptor implements InstanceConstructorInterceptor {
@Override
public void onConstruct(EnhancedInstance objInst, Object[] allArguments) {
- CacheConfiguration cacheConfiguration = (CacheConfiguration) allArguments[0];
+ try {
+ CacheConfiguration cacheConfiguration = (CacheConfiguration) allArguments[0];
- // get cache name
- if (cacheConfiguration != null) {
- objInst.setSkyWalkingDynamicField(new EhcacheEnhanceInfo(cacheConfiguration.getName()));
+ // get cache name
+ if (cacheConfiguration != null) {
+ objInst.setSkyWalkingDynamicField(new EhcacheEnhanceInfo(cacheConfiguration.getName()));
+ }
+ } catch (ClassCastException e) {
Review comment:
ok. Thank you for your seriousness
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] codecov-io edited a comment on issue #4379: Fix ehcache: missing interceptor of private constructor and setName method
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #4379: Fix ehcache: missing interceptor of private constructor and setName method
URL: https://github.com/apache/skywalking/pull/4379#issuecomment-588035622
# [Codecov](https://codecov.io/gh/apache/skywalking/pull/4379?src=pr&el=h1) Report
> Merging [#4379](https://codecov.io/gh/apache/skywalking/pull/4379?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/54c1cd5cb04f012ff48c8b87c11df11041e3f499?src=pr&el=desc) will **increase** coverage by `0.12%`.
> The diff coverage is `85.71%`.
[](https://codecov.io/gh/apache/skywalking/pull/4379?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #4379 +/- ##
==========================================
+ Coverage 26.21% 26.34% +0.12%
==========================================
Files 1183 1184 +1
Lines 27014 27015 +1
Branches 3725 3724 -1
==========================================
+ Hits 7083 7118 +35
+ Misses 19292 19259 -33
+ Partials 639 638 -1
```
| [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4379?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...plugin/ehcache/v2/EhcacheCacheNameInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4379/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXNkay1wbHVnaW4vZWhjYWNoZS0yLngtcGx1Z2luL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9wbHVnaW4vZWhjYWNoZS92Mi9FaGNhY2hlQ2FjaGVOYW1lSW50ZXJjZXB0b3IuamF2YQ==) | `100% <100%> (ø)` | |
| [...p/server/core/profile/analyze/ProfileAnalyzer.java](https://codecov.io/gh/apache/skywalking/pull/4379/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9vYXAvc2VydmVyL2NvcmUvcHJvZmlsZS9hbmFseXplL1Byb2ZpbGVBbmFseXplci5qYXZh) | `65.95% <100%> (ø)` | :arrow_up: |
| [...cache/v2/EhcachePrivateConstructorInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4379/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXNkay1wbHVnaW4vZWhjYWNoZS0yLngtcGx1Z2luL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9wbHVnaW4vZWhjYWNoZS92Mi9FaGNhY2hlUHJpdmF0ZUNvbnN0cnVjdG9ySW50ZXJjZXB0b3IuamF2YQ==) | `75% <75%> (ø)` | |
| [.../agent/core/context/trace/AbstractTracingSpan.java](https://codecov.io/gh/apache/skywalking/pull/4379/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC90cmFjZS9BYnN0cmFjdFRyYWNpbmdTcGFuLmphdmE=) | `61.6% <0%> (+0.89%)` | :arrow_up: |
| [...king/apm/agent/core/remote/GRPCChannelManager.java](https://codecov.io/gh/apache/skywalking/pull/4379/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvcmVtb3RlL0dSUENDaGFubmVsTWFuYWdlci5qYXZh) | `69.23% <0%> (+1.28%)` | :arrow_up: |
| [...m/agent/core/remote/TraceSegmentServiceClient.java](https://codecov.io/gh/apache/skywalking/pull/4379/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvcmVtb3RlL1RyYWNlU2VnbWVudFNlcnZpY2VDbGllbnQuamF2YQ==) | `79.45% <0%> (+1.36%)` | :arrow_up: |
| [.../core/remote/ServiceAndEndpointRegisterClient.java](https://codecov.io/gh/apache/skywalking/pull/4379/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvcmVtb3RlL1NlcnZpY2VBbmRFbmRwb2ludFJlZ2lzdGVyQ2xpZW50LmphdmE=) | `33.67% <0%> (+3.06%)` | :arrow_up: |
| [...alking/apm/agent/core/remote/AgentIDDecorator.java](https://codecov.io/gh/apache/skywalking/pull/4379/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvcmVtb3RlL0FnZW50SUREZWNvcmF0b3IuamF2YQ==) | `85.18% <0%> (+18.51%)` | :arrow_up: |
| ... and [2 more](https://codecov.io/gh/apache/skywalking/pull/4379/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4379?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/skywalking/pull/4379?src=pr&el=footer). Last update [54c1cd5...b2ad634](https://codecov.io/gh/apache/skywalking/pull/4379?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] lsyf commented on issue #4379: Fix ehcache: missing interceptor of private constructor and setName method
Posted by GitBox <gi...@apache.org>.
lsyf commented on issue #4379: Fix ehcache: missing interceptor of private constructor and setName method
URL: https://github.com/apache/skywalking/pull/4379#issuecomment-587506869
The weird thing is that I haven't changed the test cases and they will go wrong.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] codecov-io commented on issue #4379: Fix ehcache: missing interceptor of private constructor and setName method
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #4379: Fix ehcache: missing interceptor of private constructor and setName method
URL: https://github.com/apache/skywalking/pull/4379#issuecomment-588035622
# [Codecov](https://codecov.io/gh/apache/skywalking/pull/4379?src=pr&el=h1) Report
> Merging [#4379](https://codecov.io/gh/apache/skywalking/pull/4379?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/54c1cd5cb04f012ff48c8b87c11df11041e3f499?src=pr&el=desc) will **increase** coverage by `0.01%`.
> The diff coverage is `85.71%`.
[](https://codecov.io/gh/apache/skywalking/pull/4379?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #4379 +/- ##
==========================================
+ Coverage 26.21% 26.23% +0.01%
==========================================
Files 1183 1184 +1
Lines 27014 27015 +1
Branches 3725 3724 -1
==========================================
+ Hits 7083 7088 +5
+ Misses 19292 19288 -4
Partials 639 639
```
| [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4379?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...plugin/ehcache/v2/EhcacheCacheNameInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4379/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXNkay1wbHVnaW4vZWhjYWNoZS0yLngtcGx1Z2luL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9wbHVnaW4vZWhjYWNoZS92Mi9FaGNhY2hlQ2FjaGVOYW1lSW50ZXJjZXB0b3IuamF2YQ==) | `100% <100%> (ø)` | |
| [...p/server/core/profile/analyze/ProfileAnalyzer.java](https://codecov.io/gh/apache/skywalking/pull/4379/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9vYXAvc2VydmVyL2NvcmUvcHJvZmlsZS9hbmFseXplL1Byb2ZpbGVBbmFseXplci5qYXZh) | `65.95% <100%> (ø)` | :arrow_up: |
| [...cache/v2/EhcachePrivateConstructorInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4379/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXNkay1wbHVnaW4vZWhjYWNoZS0yLngtcGx1Z2luL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9wbHVnaW4vZWhjYWNoZS92Mi9FaGNhY2hlUHJpdmF0ZUNvbnN0cnVjdG9ySW50ZXJjZXB0b3IuamF2YQ==) | `75% <75%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4379?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/skywalking/pull/4379?src=pr&el=footer). Last update [54c1cd5...b2ad634](https://codecov.io/gh/apache/skywalking/pull/4379?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on issue #4379: Fix ehcache: missing interceptor of private constructor called by clone method
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4379: Fix ehcache: missing interceptor of private constructor called by clone method
URL: https://github.com/apache/skywalking/pull/4379#issuecomment-587288875
I think this fix needs to be improved. The reason of `#clone` not working is not caused by it is a private method, it is because the original intercept rule is targeting construct with specific parameter.
Notice this
```java
@Override
public ConstructorInterceptPoint[] getConstructorsInterceptPoints() {
return new ConstructorInterceptPoint[] {
new ConstructorInterceptPoint() {
@Override
public ElementMatcher<MethodDescription> getConstructorMatcher() {
return takesArgument(0, named("net.sf.ehcache.config.CacheConfiguration"));
}
@Override
public String getConstructorInterceptor() {
return CONSTRUCTOR_CLASS_INTERCEPT_CLASS;
}
}
};
}
```
So basically, the better fix solution is add another constructor interceptor point to target the private constructor, I assume this one `private Cache(Cache original) throws CloneNotSupportedException`?
@lsyf What do you think?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on issue #4379: Fix ehcache: missing interceptor of private constructor and setName method
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4379: Fix ehcache: missing interceptor of private constructor and setName method
URL: https://github.com/apache/skywalking/pull/4379#issuecomment-588202362
@mrproliu Please don't use `merge` directly. because of this, this contributor wouldn't be counted, and the commit log breaks with multiple details commits.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] lsyf commented on issue #4379: Fix ehcache: missing interceptor of private constructor and setName method
Posted by GitBox <gi...@apache.org>.
lsyf commented on issue #4379: Fix ehcache: missing interceptor of private constructor and setName method
URL: https://github.com/apache/skywalking/pull/4379#issuecomment-587496796
> I think this fix needs to be improved. The reason of `#clone` not working is not caused by it is a private method, it is because the original intercept rule is targeting construct with specific parameter.
>
> Notice this
>
> ```java
> @Override
> public ConstructorInterceptPoint[] getConstructorsInterceptPoints() {
> return new ConstructorInterceptPoint[] {
> new ConstructorInterceptPoint() {
> @Override
> public ElementMatcher<MethodDescription> getConstructorMatcher() {
> return takesArgument(0, named("net.sf.ehcache.config.CacheConfiguration"));
> }
>
> @Override
> public String getConstructorInterceptor() {
> return CONSTRUCTOR_CLASS_INTERCEPT_CLASS;
> }
> }
> };
> }
> ```
>
> So basically, the better fix solution is add another constructor interceptor point to target the private constructor, I assume this one `private Cache(Cache original) throws CloneNotSupportedException`?
>
> @lsyf What do you think?
yes. Beacause the purpose of the interceptor is to save cache name，`#setName` also need to be intercepted
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4379: Fix ehcache: missing interceptor of private constructor and setName method
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4379: Fix ehcache: missing interceptor of private constructor and setName method
URL: https://github.com/apache/skywalking/pull/4379#discussion_r381049791
##########
File path: apm-sniffer/apm-sdk-plugin/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheConstructorInterceptor.java
##########
@@ -18,18 +18,28 @@
package org.apache.skywalking.apm.plugin.ehcache.v2;
+import net.sf.ehcache.Cache;
import net.sf.ehcache.config.CacheConfiguration;
import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceConstructorInterceptor;
public class EhcacheConstructorInterceptor implements InstanceConstructorInterceptor {
@Override
public void onConstruct(EnhancedInstance objInst, Object[] allArguments) {
- CacheConfiguration cacheConfiguration = (CacheConfiguration) allArguments[0];
+ try {
+ CacheConfiguration cacheConfiguration = (CacheConfiguration) allArguments[0];
- // get cache name
- if (cacheConfiguration != null) {
- objInst.setSkyWalkingDynamicField(new EhcacheEnhanceInfo(cacheConfiguration.getName()));
+ // get cache name
+ if (cacheConfiguration != null) {
+ objInst.setSkyWalkingDynamicField(new EhcacheEnhanceInfo(cacheConfiguration.getName()));
+ }
+ } catch (ClassCastException e) {
Review comment:
Why? `instanceof` is still much much slower than instrumentation level match.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4379: Fix ehcache: missing interceptor of private constructor and setName method
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4379: Fix ehcache: missing interceptor of private constructor and setName method
URL: https://github.com/apache/skywalking/pull/4379#discussion_r381014400
##########
File path: apm-sniffer/apm-sdk-plugin/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheConstructorInterceptor.java
##########
@@ -18,18 +18,28 @@
package org.apache.skywalking.apm.plugin.ehcache.v2;
+import net.sf.ehcache.Cache;
import net.sf.ehcache.config.CacheConfiguration;
import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceConstructorInterceptor;
public class EhcacheConstructorInterceptor implements InstanceConstructorInterceptor {
@Override
public void onConstruct(EnhancedInstance objInst, Object[] allArguments) {
- CacheConfiguration cacheConfiguration = (CacheConfiguration) allArguments[0];
+ try {
+ CacheConfiguration cacheConfiguration = (CacheConfiguration) allArguments[0];
- // get cache name
- if (cacheConfiguration != null) {
- objInst.setSkyWalkingDynamicField(new EhcacheEnhanceInfo(cacheConfiguration.getName()));
+ // get cache name
+ if (cacheConfiguration != null) {
+ objInst.setSkyWalkingDynamicField(new EhcacheEnhanceInfo(cacheConfiguration.getName()));
+ }
+ } catch (ClassCastException e) {
Review comment:
You don't need to use this `try/catch`. `getConstructorsInterceptPoints` returns an array, you should set up the interceptors separately. This one you didi has a performance issue.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on issue #4379: Fix ehcache: missing interceptor of private constructor called by clone method
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4379: Fix ehcache: missing interceptor of private constructor called by clone method
URL: https://github.com/apache/skywalking/pull/4379#issuecomment-587214239
`/test/plugin/scenarios/ehcache-2.x-scenario` needs to be updated too, read [plugin test doc](https://github.com/apache/skywalking/blob/master/docs/en/guides/Plugin-test.md) to get more details. This is an easy update.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] lsyf commented on a change in pull request #4379: Fix ehcache: missing interceptor of private constructor and setName method
Posted by GitBox <gi...@apache.org>.
lsyf commented on a change in pull request #4379: Fix ehcache: missing interceptor of private constructor and setName method
URL: https://github.com/apache/skywalking/pull/4379#discussion_r381049349
##########
File path: apm-sniffer/apm-sdk-plugin/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/EhcacheConstructorInterceptor.java
##########
@@ -18,18 +18,28 @@
package org.apache.skywalking.apm.plugin.ehcache.v2;
+import net.sf.ehcache.Cache;
import net.sf.ehcache.config.CacheConfiguration;
import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceConstructorInterceptor;
public class EhcacheConstructorInterceptor implements InstanceConstructorInterceptor {
@Override
public void onConstruct(EnhancedInstance objInst, Object[] allArguments) {
- CacheConfiguration cacheConfiguration = (CacheConfiguration) allArguments[0];
+ try {
+ CacheConfiguration cacheConfiguration = (CacheConfiguration) allArguments[0];
- // get cache name
- if (cacheConfiguration != null) {
- objInst.setSkyWalkingDynamicField(new EhcacheEnhanceInfo(cacheConfiguration.getName()));
+ // get cache name
+ if (cacheConfiguration != null) {
+ objInst.setSkyWalkingDynamicField(new EhcacheEnhanceInfo(cacheConfiguration.getName()));
+ }
+ } catch (ClassCastException e) {
Review comment:
Can I use `instanseof` instead?
```java
if (argument instanceof CacheConfiguration) {
CacheConfiguration cacheConfiguration = (CacheConfiguration) argument;
// get cache name
if (cacheConfiguration != null) {
objInst.setSkyWalkingDynamicField(new EhcacheEnhanceInfo(cacheConfiguration.getName()));
}
} else if (argument instanceof Cache) {
Cache cache = (Cache) argument;
// get cache name
if (cache != null && cache.getCacheConfiguration() != null) {
objInst.setSkyWalkingDynamicField(new EhcacheEnhanceInfo(cache.getCacheConfiguration().getName()));
}
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] lsyf commented on a change in pull request #4379: Fix ehcache: missing interceptor of private constructor and setName method
Posted by GitBox <gi...@apache.org>.
lsyf commented on a change in pull request #4379: Fix ehcache: missing interceptor of private constructor and setName method
URL: https://github.com/apache/skywalking/pull/4379#discussion_r381125588
##########
File path: apm-sniffer/apm-sdk-plugin/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/define/EhcachePluginInstrumentation.java
##########
@@ -82,13 +88,41 @@
public String getConstructorInterceptor() {
return CONSTRUCTOR_CLASS_INTERCEPT_CLASS;
}
+ },
+ new ConstructorInterceptPoint() {
+ @Override
+ public ElementMatcher<MethodDescription> getConstructorMatcher() {
+ return isPrivate().and(takesArgument(0, named("net.sf.ehcache.Cache")));
+ }
+
+ @Override
+ public String getConstructorInterceptor() {
+ return PRIVATE_CONSTRUCTOR_CLASS_INTERCEPT_CLASS;
+ }
}
};
}
@Override
public InstanceMethodsInterceptPoint[] getInstanceMethodsInterceptPoints() {
return new InstanceMethodsInterceptPoint[] {
+ new InstanceMethodsInterceptPoint() {
+ @Override
+ public ElementMatcher<MethodDescription> getMethodsMatcher() {
+ return named(CACHE_NAME_ENHANCE_METHOD).and(takesArgument(0, String.class));
+ }
+
+ @Override
+ public String getMethodsInterceptor() {
+ return CACHE_NAME_INTERCEPTOR_CLASS;
+ }
+
+ @Override
+ public boolean isOverrideArgs() {
+ return false;
+ }
Review comment:
Yes. Do I need to format and check?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] mrproliu commented on a change in pull request #4379: Fix ehcache: missing interceptor of private constructor and setName method
Posted by GitBox <gi...@apache.org>.
mrproliu commented on a change in pull request #4379: Fix ehcache: missing interceptor of private constructor and setName method
URL: https://github.com/apache/skywalking/pull/4379#discussion_r381121129
##########
File path: apm-sniffer/apm-sdk-plugin/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/define/EhcachePluginInstrumentation.java
##########
@@ -82,13 +88,41 @@
public String getConstructorInterceptor() {
return CONSTRUCTOR_CLASS_INTERCEPT_CLASS;
}
+ },
+ new ConstructorInterceptPoint() {
+ @Override
+ public ElementMatcher<MethodDescription> getConstructorMatcher() {
+ return isPrivate().and(takesArgument(0, named("net.sf.ehcache.Cache")));
+ }
+
+ @Override
+ public String getConstructorInterceptor() {
+ return PRIVATE_CONSTRUCTOR_CLASS_INTERCEPT_CLASS;
+ }
}
};
}
@Override
public InstanceMethodsInterceptPoint[] getInstanceMethodsInterceptPoints() {
return new InstanceMethodsInterceptPoint[] {
+ new InstanceMethodsInterceptPoint() {
+ @Override
+ public ElementMatcher<MethodDescription> getMethodsMatcher() {
+ return named(CACHE_NAME_ENHANCE_METHOD).and(takesArgument(0, String.class));
+ }
+
+ @Override
+ public String getMethodsInterceptor() {
+ return CACHE_NAME_INTERCEPTOR_CLASS;
+ }
+
+ @Override
+ public boolean isOverrideArgs() {
+ return false;
+ }
Review comment:
Is this a code format error?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4379: Fix ehcache: missing interceptor of private constructor and setName method
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4379: Fix ehcache: missing interceptor of private constructor and setName method
URL: https://github.com/apache/skywalking/pull/4379#discussion_r381154849
##########
File path: apm-sniffer/apm-sdk-plugin/ehcache-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/ehcache/v2/define/EhcachePluginInstrumentation.java
##########
@@ -82,13 +88,41 @@
public String getConstructorInterceptor() {
return CONSTRUCTOR_CLASS_INTERCEPT_CLASS;
}
+ },
+ new ConstructorInterceptPoint() {
+ @Override
+ public ElementMatcher<MethodDescription> getConstructorMatcher() {
+ return isPrivate().and(takesArgument(0, named("net.sf.ehcache.Cache")));
+ }
+
+ @Override
+ public String getConstructorInterceptor() {
+ return PRIVATE_CONSTRUCTOR_CLASS_INTERCEPT_CLASS;
+ }
}
};
}
@Override
public InstanceMethodsInterceptPoint[] getInstanceMethodsInterceptPoints() {
return new InstanceMethodsInterceptPoint[] {
+ new InstanceMethodsInterceptPoint() {
+ @Override
+ public ElementMatcher<MethodDescription> getMethodsMatcher() {
+ return named(CACHE_NAME_ENHANCE_METHOD).and(takesArgument(0, String.class));
+ }
+
+ @Override
+ public String getMethodsInterceptor() {
+ return CACHE_NAME_INTERCEPTOR_CLASS;
+ }
+
+ @Override
+ public boolean isOverrideArgs() {
+ return false;
+ }
Review comment:
Yes, you should. The format check will be more strict soon for the test case.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] lsyf commented on issue #4379: Fix ehcache: missing interceptor of private constructor and setName method
Posted by GitBox <gi...@apache.org>.
lsyf commented on issue #4379: Fix ehcache: missing interceptor of private constructor and setName method
URL: https://github.com/apache/skywalking/pull/4379#issuecomment-587502984
> Also, your plugin test is fail, please test locally.
sorry. My computer operating system is windows. I cannot install docker due to company security software restrictions. So I can only check by myself to make sure there are no errors
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on issue #4379: Fix ehcache: missing interceptor of private constructor called by clone method
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4379: Fix ehcache: missing interceptor of private constructor called by clone method
URL: https://github.com/apache/skywalking/pull/4379#issuecomment-587289047
Also, your plugin test is fail, please test locally.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] mrproliu merged pull request #4379: Fix ehcache: missing interceptor of private constructor and setName method
Posted by GitBox <gi...@apache.org>.
mrproliu merged pull request #4379: Fix ehcache: missing interceptor of private constructor and setName method
URL: https://github.com/apache/skywalking/pull/4379
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services