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/08 11:52:39 UTC
[GitHub] [skywalking] lxliuxuankb opened a new pull request #4327: tag
annotation support return expression
lxliuxuankb opened a new pull request #4327: tag annotation support return expression
URL: https://github.com/apache/skywalking/pull/4327
Please answer these questions before submitting pull request
- Why submit this pull request?
- [ ] Bug fix
- [ ] New feature provided
- [ ] Improve performance
- Related issues
___
### Bug fix
- Bug description.
- How to fix?
___
### New feature or improvement
- Describe the details and related test reports.
@Tag Annotation support method Return value , Issue: https://github.com/apache/skywalking/issues/4255
----------------------------------------------------------------
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] lxliuxuankb commented on issue #4327: tag annotation
support return expression
Posted by GitBox <gi...@apache.org>.
lxliuxuankb commented on issue #4327: tag annotation support return expression
URL: https://github.com/apache/skywalking/pull/4327#issuecomment-583732607
> Please fix CI first.
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.
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 #4327:
Tag annotation supports returned expression
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4327: Tag annotation supports returned expression
URL: https://github.com/apache/skywalking/pull/4327#discussion_r377671896
##########
File path: apm-sniffer/apm-toolkit-activation/apm-toolkit-trace-activation/src/main/java/org/apache/skywalking/apm/toolkit/activation/trace/TagAnnotationMethodInterceptor.java
##########
@@ -46,23 +46,47 @@ public void beforeMethod(final EnhancedInstance objInst, final Method method, fi
final Tags tags = method.getAnnotation(Tags.class);
if (tags != null && tags.value().length > 0) {
for (final Tag tag : tags.value()) {
- tagSpan(activeSpan, tag, context);
+ if (!TagUtil.isReturnTag(tag.value())) {
+ TagUtil.tagParamsSpan(activeSpan, context, tag);
+ }
}
}
final Tag tag = method.getAnnotation(Tag.class);
- if (tag != null) {
- tagSpan(activeSpan, tag, context);
+ if (tag != null && !TagUtil.isReturnTag(tag.value())) {
+ TagUtil.tagParamsSpan(activeSpan, context, tag);
}
}
- private void tagSpan(final AbstractSpan span, final Tag tag, final Map<String, Object> context) {
- new StringTag(tag.key()).set(span, CustomizeExpression.parseExpression(tag.value(), context));
- }
@Override
- public Object afterMethod(final EnhancedInstance objInst, final Method method, final Object[] allArguments,
- final Class<?>[] argumentsTypes, final Object ret) {
+ public Object afterMethod(
+ final EnhancedInstance objInst,
+ final Method method,
+ final Object[] allArguments,
+ final Class<?>[] argumentsTypes,
+ final Object ret) {
+ try {
+ if (ret == null || !ContextManager.isActive()) {
+ return ret;
+ }
+ final AbstractSpan localSpan = ContextManager.activeSpan();
+ final Map<String, Object> context = CustomizeExpression.evaluationReturnContext(ret);
+ final Tags tags = method.getAnnotation(Tags.class);
+ if (tags != null && tags.value().length > 0) {
+ for (final Tag tag : tags.value()) {
+ if (TagUtil.isReturnTag(tag.value())) {
+ TagUtil.tagReturnSpanSpan(localSpan, context, tag);
+ }
+ }
+ }
+ final Tag tag = method.getAnnotation(Tag.class);
+ if (tag != null && TagUtil.isReturnTag(tag.value())) {
+ TagUtil.tagReturnSpanSpan(localSpan, context, tag);
+ }
+ } finally {
+ ContextManager.stopSpan();
Review comment:
Sorry, this is not right. This could be NPE, if `ContextManager.isActive() == false`, right? In `#beforeMethod`, there is
```java
if (!ContextManager.isActive()) {
return;
}
```
----------------------------------------------------------------
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] kezhenxu94 commented on a change in pull request
#4327: Tag annotation supports returned expression
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #4327: Tag annotation supports returned expression
URL: https://github.com/apache/skywalking/pull/4327#discussion_r377997459
##########
File path: apm-sniffer/apm-toolkit-activation/apm-toolkit-trace-activation/src/main/java/org/apache/skywalking/apm/toolkit/activation/trace/TagAnnotationMethodInterceptor.java
##########
@@ -46,23 +46,47 @@ public void beforeMethod(final EnhancedInstance objInst, final Method method, fi
final Tags tags = method.getAnnotation(Tags.class);
if (tags != null && tags.value().length > 0) {
for (final Tag tag : tags.value()) {
- tagSpan(activeSpan, tag, context);
+ if (!TagUtil.isReturnTag(tag.value())) {
+ TagUtil.tagParamsSpan(activeSpan, context, tag);
+ }
}
}
final Tag tag = method.getAnnotation(Tag.class);
- if (tag != null) {
- tagSpan(activeSpan, tag, context);
+ if (tag != null && !TagUtil.isReturnTag(tag.value())) {
+ TagUtil.tagParamsSpan(activeSpan, context, tag);
}
}
- private void tagSpan(final AbstractSpan span, final Tag tag, final Map<String, Object> context) {
- new StringTag(tag.key()).set(span, CustomizeExpression.parseExpression(tag.value(), context));
- }
@Override
- public Object afterMethod(final EnhancedInstance objInst, final Method method, final Object[] allArguments,
- final Class<?>[] argumentsTypes, final Object ret) {
+ public Object afterMethod(
+ final EnhancedInstance objInst,
+ final Method method,
+ final Object[] allArguments,
+ final Class<?>[] argumentsTypes,
+ final Object ret) {
+ try {
+ if (ret == null || !ContextManager.isActive()) {
+ return ret;
+ }
+ final AbstractSpan localSpan = ContextManager.activeSpan();
+ final Map<String, Object> context = CustomizeExpression.evaluationReturnContext(ret);
+ final Tags tags = method.getAnnotation(Tags.class);
+ if (tags != null && tags.value().length > 0) {
+ for (final Tag tag : tags.value()) {
+ if (TagUtil.isReturnTag(tag.value())) {
+ TagUtil.tagReturnSpanSpan(localSpan, context, tag);
+ }
+ }
+ }
+ final Tag tag = method.getAnnotation(Tag.class);
+ if (tag != null && TagUtil.isReturnTag(tag.value())) {
+ TagUtil.tagReturnSpanSpan(localSpan, context, tag);
+ }
+ } finally {
+ ContextManager.stopSpan();
Review comment:
We don't create span in the `#beforeMethod`, so please remove the `stopSpan()` here, we cannot stop a span by mistake here
----------------------------------------------------------------
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] lxliuxuankb commented on a change in pull request
#4327: tag annotation support return expression
Posted by GitBox <gi...@apache.org>.
lxliuxuankb commented on a change in pull request #4327: tag annotation support return expression
URL: https://github.com/apache/skywalking/pull/4327#discussion_r376768887
##########
File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/util/TagUtil.java
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.agent.core.context.util;
+
+import java.util.Map;
+
+import org.apache.skywalking.apm.agent.core.context.tag.StringTag;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.util.CustomizeExpression;
+
+public class TagUtil {
+ public static void tagParamsSpan(final AbstractSpan span, final Map<String, Object> context,
+ String key, String value) {
+ new StringTag(key).set(span, CustomizeExpression.parseExpression(value, context));
+ }
+
+ public static void tagReturnSpanSpan(final AbstractSpan span, final Map<String, Object> context,
+ String key, String value) {
+ new StringTag(key).set(span, CustomizeExpression.parseReturnExpression(value, context));
+ }
+
+ public static Boolean isReturnTag(String expression) {
+ String[] es = expression.split("\\.");
+ return es.length == 2 && "returnedObj".equals(es[0]);
Review comment:
because expression just support string like returnedObj.xxxxx
----------------------------------------------------------------
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 #4327: tag annotation
support return expression
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4327: tag annotation support return expression
URL: https://github.com/apache/skywalking/pull/4327#issuecomment-583839961
@lxliuxuankb Please follow this
> Toolkit plugin test should be updated 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.
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 #4327: Tag annotation
supports returned expression
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4327: Tag annotation supports returned expression
URL: https://github.com/apache/skywalking/pull/4327#issuecomment-584539033
The strict format rule has been merged, please update this PR.
----------------------------------------------------------------
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] lxliuxuankb commented on a change in pull request
#4327: tag annotation support return expression
Posted by GitBox <gi...@apache.org>.
lxliuxuankb commented on a change in pull request #4327: tag annotation support return expression
URL: https://github.com/apache/skywalking/pull/4327#discussion_r376769032
##########
File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/util/TagUtil.java
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.agent.core.context.util;
+
+import java.util.Map;
+
+import org.apache.skywalking.apm.agent.core.context.tag.StringTag;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.util.CustomizeExpression;
+
+public class TagUtil {
Review comment:
ok ,i will fix this
----------------------------------------------------------------
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] kezhenxu94 merged pull request #4327: Tag annotation
supports returned expression
Posted by GitBox <gi...@apache.org>.
kezhenxu94 merged pull request #4327: Tag annotation supports returned expression
URL: https://github.com/apache/skywalking/pull/4327
----------------------------------------------------------------
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 #4327:
tag annotation support return expression
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4327: tag annotation support return expression
URL: https://github.com/apache/skywalking/pull/4327#discussion_r376709866
##########
File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/util/TagUtil.java
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.agent.core.context.util;
+
+import java.util.Map;
+
+import org.apache.skywalking.apm.agent.core.context.tag.StringTag;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.util.CustomizeExpression;
+
+public class TagUtil {
+ public static void tagParamsSpan(final AbstractSpan span, final Map<String, Object> context,
+ String key, String value) {
+ new StringTag(key).set(span, CustomizeExpression.parseExpression(value, context));
+ }
+
+ public static void tagReturnSpanSpan(final AbstractSpan span, final Map<String, Object> context,
+ String key, String value) {
+ new StringTag(key).set(span, CustomizeExpression.parseReturnExpression(value, context));
+ }
+
+ public static Boolean isReturnTag(String expression) {
+ String[] es = expression.split("\\.");
+ return es.length == 2 && "returnedObj".equals(es[0]);
Review comment:
Why `length==2` is required?
----------------------------------------------------------------
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 #4327: tag
annotation support return expression
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #4327: tag annotation support return expression
URL: https://github.com/apache/skywalking/pull/4327#issuecomment-583735666
# [Codecov](https://codecov.io/gh/apache/skywalking/pull/4327?src=pr&el=h1) Report
> Merging [#4327](https://codecov.io/gh/apache/skywalking/pull/4327?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/eb680cb3cc1e5962d9553cbc13e62af64fe878b8?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `93.75%`.
[](https://codecov.io/gh/apache/skywalking/pull/4327?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #4327 +/- ##
======================================
Coverage 27.1% 27.1%
======================================
Files 1177 1177
Lines 25723 25723
Branches 3679 3679
======================================
Hits 6973 6973
Misses 18130 18130
Partials 620 620
```
| [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4327?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [.../core/profile/analyze/ProfileAnalyzeCollector.java](https://codecov.io/gh/apache/skywalking/pull/4327/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9vYXAvc2VydmVyL2NvcmUvcHJvZmlsZS9hbmFseXplL1Byb2ZpbGVBbmFseXplQ29sbGVjdG9yLmphdmE=) | `100% <ø> (ø)` | :arrow_up: |
| [...lking/oap/query/graphql/resolver/ProfileQuery.java](https://codecov.io/gh/apache/skywalking/pull/4327/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItcXVlcnktcGx1Z2luL3F1ZXJ5LWdyYXBocWwtcGx1Z2luL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL29hcC9xdWVyeS9ncmFwaHFsL3Jlc29sdmVyL1Byb2ZpbGVRdWVyeS5qYXZh) | `0% <0%> (ø)` | :arrow_up: |
| [.../server/core/profile/analyze/ProfileStackNode.java](https://codecov.io/gh/apache/skywalking/pull/4327/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9vYXAvc2VydmVyL2NvcmUvcHJvZmlsZS9hbmFseXplL1Byb2ZpbGVTdGFja05vZGUuamF2YQ==) | `90.38% <100%> (ø)` | :arrow_up: |
| [...p/server/core/profile/analyze/ProfileAnalyzer.java](https://codecov.io/gh/apache/skywalking/pull/4327/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9vYXAvc2VydmVyL2NvcmUvcHJvZmlsZS9hbmFseXplL1Byb2ZpbGVBbmFseXplci5qYXZh) | `77.77% <100%> (ø)` | :arrow_up: |
| [...oap/server/core/query/entity/ProfileStackTree.java](https://codecov.io/gh/apache/skywalking/pull/4327/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9vYXAvc2VydmVyL2NvcmUvcXVlcnkvZW50aXR5L1Byb2ZpbGVTdGFja1RyZWUuamF2YQ==) | `100% <100%> (ø)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4327?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/4327?src=pr&el=footer). Last update [eb680cb...8e5b8ba](https://codecov.io/gh/apache/skywalking/pull/4327?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] codecov-io edited a comment on issue #4327: tag
annotation support return expression
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #4327: tag annotation support return expression
URL: https://github.com/apache/skywalking/pull/4327#issuecomment-583735666
# [Codecov](https://codecov.io/gh/apache/skywalking/pull/4327?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@b5cd69d`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `22.87%`.
[](https://codecov.io/gh/apache/skywalking/pull/4327?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #4327 +/- ##
=========================================
Coverage ? 27.06%
=========================================
Files ? 1176
Lines ? 25718
Branches ? 3678
=========================================
Hits ? 6960
Misses ? 18139
Partials ? 619
```
| [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4327?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...p/server/core/profile/analyze/ProfileAnalyzer.java](https://codecov.io/gh/apache/skywalking/pull/4327/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9vYXAvc2VydmVyL2NvcmUvcHJvZmlsZS9hbmFseXplL1Byb2ZpbGVBbmFseXplci5qYXZh) | `77.77% <ø> (ø)` | |
| [...walking/oap/server/core/storage/StorageModule.java](https://codecov.io/gh/apache/skywalking/pull/4327/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9vYXAvc2VydmVyL2NvcmUvc3RvcmFnZS9TdG9yYWdlTW9kdWxlLmphdmE=) | `0% <ø> (ø)` | |
| [...kywalking/apm/agent/core/context/util/TagUtil.java](https://codecov.io/gh/apache/skywalking/pull/4327/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvY29udGV4dC91dGlsL1RhZ1V0aWwuamF2YQ==) | `0% <ø> (ø)` | |
| [.../oap/server/core/profile/analyze/ProfileStack.java](https://codecov.io/gh/apache/skywalking/pull/4327/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9vYXAvc2VydmVyL2NvcmUvcHJvZmlsZS9hbmFseXplL1Byb2ZpbGVTdGFjay5qYXZh) | `8.33% <ø> (ø)` | |
| [...rver/core/profile/ProfileThreadSnapshotRecord.java](https://codecov.io/gh/apache/skywalking/pull/4327/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9vYXAvc2VydmVyL2NvcmUvcHJvZmlsZS9Qcm9maWxlVGhyZWFkU25hcHNob3RSZWNvcmQuamF2YQ==) | `0% <0%> (ø)` | |
| [...csearch/query/ProfileThreadSnapshotQueryEsDAO.java](https://codecov.io/gh/apache/skywalking/pull/4327/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItc3RvcmFnZS1wbHVnaW4vc3RvcmFnZS1lbGFzdGljc2VhcmNoLXBsdWdpbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9vYXAvc2VydmVyL3N0b3JhZ2UvcGx1Z2luL2VsYXN0aWNzZWFyY2gvcXVlcnkvUHJvZmlsZVRocmVhZFNuYXBzaG90UXVlcnlFc0RBTy5qYXZh) | `0% <0%> (ø)` | |
| [...n/jdbc/h2/dao/H2ProfileThreadSnapshotQueryDAO.java](https://codecov.io/gh/apache/skywalking/pull/4327/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItc3RvcmFnZS1wbHVnaW4vc3RvcmFnZS1qZGJjLWhpa2FyaWNwLXBsdWdpbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9vYXAvc2VydmVyL3N0b3JhZ2UvcGx1Z2luL2pkYmMvaDIvZGFvL0gyUHJvZmlsZVRocmVhZFNuYXBzaG90UXVlcnlEQU8uamF2YQ==) | `0% <0%> (ø)` | |
| [...gent/core/profile/ProfileTaskExecutionContext.java](https://codecov.io/gh/apache/skywalking/pull/4327/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvcHJvZmlsZS9Qcm9maWxlVGFza0V4ZWN1dGlvbkNvbnRleHQuamF2YQ==) | `0% <0%> (ø)` | |
| [...lking/oap/query/graphql/resolver/ProfileQuery.java](https://codecov.io/gh/apache/skywalking/pull/4327/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItcXVlcnktcGx1Z2luL3F1ZXJ5LWdyYXBocWwtcGx1Z2luL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL29hcC9xdWVyeS9ncmFwaHFsL3Jlc29sdmVyL1Byb2ZpbGVRdWVyeS5qYXZh) | `0% <0%> (ø)` | |
| [...icsearch7/StorageModuleElasticsearch7Provider.java](https://codecov.io/gh/apache/skywalking/pull/4327/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItc3RvcmFnZS1wbHVnaW4vc3RvcmFnZS1lbGFzdGljc2VhcmNoNy1wbHVnaW4vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvb2FwL3NlcnZlci9zdG9yYWdlL3BsdWdpbi9lbGFzdGljc2VhcmNoNy9TdG9yYWdlTW9kdWxlRWxhc3RpY3NlYXJjaDdQcm92aWRlci5qYXZh) | `0% <0%> (ø)` | |
| ... and [12 more](https://codecov.io/gh/apache/skywalking/pull/4327/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4327?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/4327?src=pr&el=footer). Last update [b5cd69d...a99ccb1](https://codecov.io/gh/apache/skywalking/pull/4327?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 #4327: tag annotation
support return expression
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4327: tag annotation support return expression
URL: https://github.com/apache/skywalking/pull/4327#issuecomment-583732380
Please fix CI 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.
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 #4327: Tag annotation
supports returned expression
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4327: Tag annotation supports returned expression
URL: https://github.com/apache/skywalking/pull/4327#issuecomment-584990917
Look like CIs are failing. Please check locally before you push to the upstream. It will cause the CPU of CI resource pool, which will block other PRs processes.
----------------------------------------------------------------
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 #4327: tag annotation
support return expression
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4327: tag annotation support return expression
URL: https://github.com/apache/skywalking/pull/4327#issuecomment-583924267
I think you don't follow what I mean. The toolkit plugin test is https://github.com/apache/skywalking/tree/master/test/plugin/scenarios/apm-toolkit-trace-scenario. Please follow it.
Plugin test doc is https://github.com/apache/skywalking/blob/master/docs/en/guides/Plugin-test.md
----------------------------------------------------------------
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 #4327:
Tag annotation supports returned expression
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4327: Tag annotation supports returned expression
URL: https://github.com/apache/skywalking/pull/4327#discussion_r377671986
##########
File path: apm-sniffer/apm-toolkit-activation/apm-toolkit-trace-activation/src/main/java/org/apache/skywalking/apm/toolkit/activation/trace/TraceAnnotationMethodInterceptor.java
##########
@@ -56,24 +56,41 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
final Tags tags = method.getAnnotation(Tags.class);
if (tags != null && tags.value().length > 0) {
for (final Tag tag : tags.value()) {
- tagSpan(localSpan, tag, context);
+ if (!TagUtil.isReturnTag(tag.value())) {
+ TagUtil.tagParamsSpan(localSpan, context, tag);
+ }
}
}
-
final Tag tag = method.getAnnotation(Tag.class);
- if (tag != null) {
- tagSpan(localSpan, tag, context);
+ if (tag != null && !TagUtil.isReturnTag(tag.value())) {
+ TagUtil.tagParamsSpan(localSpan, context, tag);
}
}
- private void tagSpan(final AbstractSpan span, final Tag tag, final Map<String, Object> context) {
- new StringTag(tag.key()).set(span, CustomizeExpression.parseExpression(tag.value(), context));
- }
-
@Override
public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes,
Object ret) throws Throwable {
- ContextManager.stopSpan();
+ try {
+ if (ret == null) {
+ return ret;
+ }
+ final AbstractSpan localSpan = ContextManager.activeSpan();
+ final Map<String, Object> context = CustomizeExpression.evaluationReturnContext(ret);
+ final Tags tags = method.getAnnotation(Tags.class);
+ if (tags != null && tags.value().length > 0) {
+ for (final Tag tag : tags.value()) {
+ if (TagUtil.isReturnTag(tag.value())) {
+ TagUtil.tagReturnSpanSpan(localSpan, context, tag);
+ }
+ }
+ }
+ final Tag tag = method.getAnnotation(Tag.class);
+ if (tag != null && TagUtil.isReturnTag(tag.value())) {
+ TagUtil.tagReturnSpanSpan(localSpan, context, tag);
+ }
+ } finally {
+ ContextManager.stopSpan();
Review comment:
Same here.
----------------------------------------------------------------
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 #4327:
Tag annotation supports returned expression
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4327: Tag annotation supports returned expression
URL: https://github.com/apache/skywalking/pull/4327#discussion_r377433331
##########
File path: apm-sniffer/apm-toolkit-activation/apm-toolkit-trace-activation/src/main/java/org/apache/skywalking/apm/toolkit/activation/util/TagUtil.java
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.toolkit.activation.util;
+
+import java.util.Map;
+
+import org.apache.skywalking.apm.agent.core.context.tag.StringTag;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.util.CustomizeExpression;
+import org.apache.skywalking.apm.toolkit.trace.Tag;
+
+public class TagUtil {
+ public static void tagParamsSpan(final AbstractSpan span, final Map<String, Object> context,
+ final Tag tag) {
+ new StringTag(tag.key()).set(span, CustomizeExpression.parseExpression(tag.value(), context));
+ }
+
+ public static void tagReturnSpanSpan(final AbstractSpan span, final Map<String, Object> context,
+ final Tag tag) {
+ new StringTag(tag.key()).set(span, CustomizeExpression.parseReturnExpression(tag.value(), context));
+ }
+
+ public static Boolean isReturnTag(String expression) {
+ String[] es = expression.split("\\.");
+ return es.length == 2 && "returnedObj".equals(es[0]);
Review comment:
`returnedObj` has a specific meaning, should be adding into the document. https://github.com/apache/skywalking/blob/master/docs/en/setup/service-agent/java-agent/Application-toolkit-trace.md
----------------------------------------------------------------
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 #4327: Tag annotation
supports returned expression
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4327: Tag annotation supports returned expression
URL: https://github.com/apache/skywalking/pull/4327#issuecomment-585046372
Welcome to be 205th contributor of this repo.
----------------------------------------------------------------
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 #4327:
tag annotation support return expression
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4327: tag annotation support return expression
URL: https://github.com/apache/skywalking/pull/4327#discussion_r376709890
##########
File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/util/TagUtil.java
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.agent.core.context.util;
+
+import java.util.Map;
+
+import org.apache.skywalking.apm.agent.core.context.tag.StringTag;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.util.CustomizeExpression;
+
+public class TagUtil {
Review comment:
Why do we need a Util at this core level?
----------------------------------------------------------------
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] lxliuxuankb commented on a change in pull request
#4327: tag annotation support return expression
Posted by GitBox <gi...@apache.org>.
lxliuxuankb commented on a change in pull request #4327: tag annotation support return expression
URL: https://github.com/apache/skywalking/pull/4327#discussion_r376768812
##########
File path: apm-sniffer/apm-toolkit-activation/apm-toolkit-trace-activation/src/main/java/org/apache/skywalking/apm/toolkit/activation/trace/TagAnnotationMethodInterceptor.java
##########
@@ -54,19 +54,18 @@ public void beforeMethod(
final Tags tags = method.getAnnotation(Tags.class);
if (tags != null && tags.value().length > 0) {
for (final Tag tag : tags.value()) {
- tagSpan(activeSpan, tag, context);
+ if (!TagUtil.isReturnTag(tag.value())) {
+ TagUtil.tagParamsSpan(activeSpan, context, tag.key(), tag.value());
+ }
}
}
final Tag tag = method.getAnnotation(Tag.class);
- if (tag != null) {
- tagSpan(activeSpan, tag, context);
+ if (tag != null && !TagUtil.isReturnTag(tag.value())) {
+ TagUtil.tagParamsSpan(activeSpan, context, tag.key(), tag.value());
}
}
- private void tagSpan(final AbstractSpan span, final Tag tag, final Map<String, Object> context) {
Review comment:
because TraceAnnotationMethodInterceptor and TagAnnotationMethodInterceptor also have this method, so move this method into TagUtil
----------------------------------------------------------------
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 #4327:
tag annotation support return expression
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4327: tag annotation support return expression
URL: https://github.com/apache/skywalking/pull/4327#discussion_r376709928
##########
File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/util/TagUtil.java
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.agent.core.context.util;
+
+import java.util.Map;
+
+import org.apache.skywalking.apm.agent.core.context.tag.StringTag;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.util.CustomizeExpression;
+
+public class TagUtil {
Review comment:
You are just changing a single plugin, don't change anything in the core level, unless it is really necessary. For this case, it doesn't seem so.
----------------------------------------------------------------
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 #4327: Tag annotation
supports returned expression
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4327: Tag annotation supports returned expression
URL: https://github.com/apache/skywalking/pull/4327#issuecomment-584985052
Once CI passed, the codes LGTM. Welcome.
----------------------------------------------------------------
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 #4327: Tag
annotation supports returned expression
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #4327: Tag annotation supports returned expression
URL: https://github.com/apache/skywalking/pull/4327#issuecomment-583735666
# [Codecov](https://codecov.io/gh/apache/skywalking/pull/4327?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@ee6364e`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `69.23%`.
[](https://codecov.io/gh/apache/skywalking/pull/4327?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #4327 +/- ##
=========================================
Coverage ? 26.41%
=========================================
Files ? 1178
Lines ? 26726
Branches ? 3695
=========================================
Hits ? 7061
Misses ? 19041
Partials ? 624
```
| [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4327?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...lking/apm/agent/core/util/CustomizeExpression.java](https://codecov.io/gh/apache/skywalking/pull/4327/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvdXRpbC9DdXN0b21pemVFeHByZXNzaW9uLmphdmE=) | `0% <0%> (ø)` | |
| [...tivation/trace/TagAnnotationMethodInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4327/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXRvb2xraXQtYWN0aXZhdGlvbi9hcG0tdG9vbGtpdC10cmFjZS1hY3RpdmF0aW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS90b29sa2l0L2FjdGl2YXRpb24vdHJhY2UvVGFnQW5ub3RhdGlvbk1ldGhvZEludGVyY2VwdG9yLmphdmE=) | `68.96% <75%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4327?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/4327?src=pr&el=footer). Last update [ee6364e...6ae8922](https://codecov.io/gh/apache/skywalking/pull/4327?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] codecov-io commented on issue #4327: tag annotation
support return expression
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #4327: tag annotation support return expression
URL: https://github.com/apache/skywalking/pull/4327#issuecomment-583735666
# [Codecov](https://codecov.io/gh/apache/skywalking/pull/4327?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@b5cd69d`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `22.87%`.
[](https://codecov.io/gh/apache/skywalking/pull/4327?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #4327 +/- ##
=========================================
Coverage ? 27.08%
=========================================
Files ? 1176
Lines ? 25718
Branches ? 3678
=========================================
Hits ? 6965
Misses ? 18135
Partials ? 618
```
| [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4327?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...p/server/core/profile/analyze/ProfileAnalyzer.java](https://codecov.io/gh/apache/skywalking/pull/4327/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9vYXAvc2VydmVyL2NvcmUvcHJvZmlsZS9hbmFseXplL1Byb2ZpbGVBbmFseXplci5qYXZh) | `77.77% <ø> (ø)` | |
| [...walking/oap/server/core/storage/StorageModule.java](https://codecov.io/gh/apache/skywalking/pull/4327/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9vYXAvc2VydmVyL2NvcmUvc3RvcmFnZS9TdG9yYWdlTW9kdWxlLmphdmE=) | `0% <ø> (ø)` | |
| [.../oap/server/core/profile/analyze/ProfileStack.java](https://codecov.io/gh/apache/skywalking/pull/4327/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9vYXAvc2VydmVyL2NvcmUvcHJvZmlsZS9hbmFseXplL1Byb2ZpbGVTdGFjay5qYXZh) | `8.33% <ø> (ø)` | |
| [...rver/core/profile/ProfileThreadSnapshotRecord.java](https://codecov.io/gh/apache/skywalking/pull/4327/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9vYXAvc2VydmVyL2NvcmUvcHJvZmlsZS9Qcm9maWxlVGhyZWFkU25hcHNob3RSZWNvcmQuamF2YQ==) | `0% <0%> (ø)` | |
| [...csearch/query/ProfileThreadSnapshotQueryEsDAO.java](https://codecov.io/gh/apache/skywalking/pull/4327/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItc3RvcmFnZS1wbHVnaW4vc3RvcmFnZS1lbGFzdGljc2VhcmNoLXBsdWdpbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9vYXAvc2VydmVyL3N0b3JhZ2UvcGx1Z2luL2VsYXN0aWNzZWFyY2gvcXVlcnkvUHJvZmlsZVRocmVhZFNuYXBzaG90UXVlcnlFc0RBTy5qYXZh) | `0% <0%> (ø)` | |
| [...gent/core/profile/ProfileTaskExecutionContext.java](https://codecov.io/gh/apache/skywalking/pull/4327/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvcHJvZmlsZS9Qcm9maWxlVGFza0V4ZWN1dGlvbkNvbnRleHQuamF2YQ==) | `0% <0%> (ø)` | |
| [...lking/oap/query/graphql/resolver/ProfileQuery.java](https://codecov.io/gh/apache/skywalking/pull/4327/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItcXVlcnktcGx1Z2luL3F1ZXJ5LWdyYXBocWwtcGx1Z2luL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL29hcC9xdWVyeS9ncmFwaHFsL3Jlc29sdmVyL1Byb2ZpbGVRdWVyeS5qYXZh) | `0% <0%> (ø)` | |
| [...icsearch7/StorageModuleElasticsearch7Provider.java](https://codecov.io/gh/apache/skywalking/pull/4327/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItc3RvcmFnZS1wbHVnaW4vc3RvcmFnZS1lbGFzdGljc2VhcmNoNy1wbHVnaW4vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvb2FwL3NlcnZlci9zdG9yYWdlL3BsdWdpbi9lbGFzdGljc2VhcmNoNy9TdG9yYWdlTW9kdWxlRWxhc3RpY3NlYXJjaDdQcm92aWRlci5qYXZh) | `0% <0%> (ø)` | |
| [...rver/storage/plugin/jdbc/h2/H2StorageProvider.java](https://codecov.io/gh/apache/skywalking/pull/4327/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItc3RvcmFnZS1wbHVnaW4vc3RvcmFnZS1qZGJjLWhpa2FyaWNwLXBsdWdpbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9vYXAvc2VydmVyL3N0b3JhZ2UvcGx1Z2luL2pkYmMvaDIvSDJTdG9yYWdlUHJvdmlkZXIuamF2YQ==) | `0% <0%> (ø)` | |
| [...le/provider/handler/ProfileTaskServiceHandler.java](https://codecov.io/gh/apache/skywalking/pull/4327/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItcmVjZWl2ZXItcGx1Z2luL3NreXdhbGtpbmctcHJvZmlsZS1yZWNlaXZlci1wbHVnaW4vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvb2FwL3NlcnZlci9yZWNlaXZlci9wcm9maWxlL3Byb3ZpZGVyL2hhbmRsZXIvUHJvZmlsZVRhc2tTZXJ2aWNlSGFuZGxlci5qYXZh) | `0% <0%> (ø)` | |
| ... and [11 more](https://codecov.io/gh/apache/skywalking/pull/4327/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4327?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/4327?src=pr&el=footer). Last update [b5cd69d...a99ccb1](https://codecov.io/gh/apache/skywalking/pull/4327?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 a change in pull request #4327:
Tag annotation supports returned expression
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4327: Tag annotation supports returned expression
URL: https://github.com/apache/skywalking/pull/4327#discussion_r377433128
##########
File path: apm-sniffer/apm-toolkit-activation/apm-toolkit-trace-activation/src/main/java/org/apache/skywalking/apm/toolkit/activation/trace/TagAnnotationMethodInterceptor.java
##########
@@ -75,6 +74,25 @@ public Object afterMethod(
final Object[] allArguments,
final Class<?>[] argumentsTypes,
final Object ret) {
+ if (ret == null || !ContextManager.isActive()) {
+ ContextManager.stopSpan();
+ return ret;
Review comment:
Move these two lines into the final block.
----------------------------------------------------------------
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 #4327: Tag annotation
supports returned expression
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4327: Tag annotation supports returned expression
URL: https://github.com/apache/skywalking/pull/4327#issuecomment-584980509
BTW, please check CIs why they are failing.
----------------------------------------------------------------
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 #4327:
tag annotation support return expression
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4327: tag annotation support return expression
URL: https://github.com/apache/skywalking/pull/4327#discussion_r376768922
##########
File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/util/TagUtil.java
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.agent.core.context.util;
+
+import java.util.Map;
+
+import org.apache.skywalking.apm.agent.core.context.tag.StringTag;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.util.CustomizeExpression;
+
+public class TagUtil {
Review comment:
But you moved the TagUtil into the core, that is not 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] kezhenxu94 commented on a change in pull request
#4327: Tag annotation supports returned expression
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #4327: Tag annotation supports returned expression
URL: https://github.com/apache/skywalking/pull/4327#discussion_r377997300
##########
File path: apm-sniffer/apm-toolkit-activation/apm-toolkit-trace-activation/src/main/java/org/apache/skywalking/apm/toolkit/activation/trace/TraceAnnotationMethodInterceptor.java
##########
@@ -56,24 +56,41 @@ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allAr
final Tags tags = method.getAnnotation(Tags.class);
if (tags != null && tags.value().length > 0) {
for (final Tag tag : tags.value()) {
- tagSpan(localSpan, tag, context);
+ if (!TagUtil.isReturnTag(tag.value())) {
+ TagUtil.tagParamsSpan(localSpan, context, tag);
+ }
}
}
-
final Tag tag = method.getAnnotation(Tag.class);
- if (tag != null) {
- tagSpan(localSpan, tag, context);
+ if (tag != null && !TagUtil.isReturnTag(tag.value())) {
+ TagUtil.tagParamsSpan(localSpan, context, tag);
}
}
- private void tagSpan(final AbstractSpan span, final Tag tag, final Map<String, Object> context) {
- new StringTag(tag.key()).set(span, CustomizeExpression.parseExpression(tag.value(), context));
- }
-
@Override
public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes,
Object ret) throws Throwable {
- ContextManager.stopSpan();
+ try {
+ if (ret == null) {
+ return ret;
+ }
+ final AbstractSpan localSpan = ContextManager.activeSpan();
+ final Map<String, Object> context = CustomizeExpression.evaluationReturnContext(ret);
+ final Tags tags = method.getAnnotation(Tags.class);
+ if (tags != null && tags.value().length > 0) {
+ for (final Tag tag : tags.value()) {
+ if (TagUtil.isReturnTag(tag.value())) {
+ TagUtil.tagReturnSpanSpan(localSpan, context, tag);
+ }
+ }
+ }
+ final Tag tag = method.getAnnotation(Tag.class);
+ if (tag != null && TagUtil.isReturnTag(tag.value())) {
+ TagUtil.tagReturnSpanSpan(localSpan, context, tag);
+ }
+ } finally {
+ ContextManager.stopSpan();
Review comment:
> Same here.
There is no conditional creation of span in the `#beforeMethod`, so it's right here
----------------------------------------------------------------
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 #4327: Tag annotation
supports returned expression
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4327: Tag annotation supports returned expression
URL: https://github.com/apache/skywalking/pull/4327#issuecomment-584541674
> let's merge this first and be more friendly
The comments are easy to fix, generally, this PR is good enough, just need a little time to shape it.
Don't worry, 5 days review is normal in any place :)
----------------------------------------------------------------
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] lxliuxuankb commented on a change in pull request
#4327: tag annotation support return expression
Posted by GitBox <gi...@apache.org>.
lxliuxuankb commented on a change in pull request #4327: tag annotation support return expression
URL: https://github.com/apache/skywalking/pull/4327#discussion_r376768816
##########
File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/util/TagUtil.java
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.agent.core.context.util;
+
+import java.util.Map;
+
+import org.apache.skywalking.apm.agent.core.context.tag.StringTag;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.util.CustomizeExpression;
+
+public class TagUtil {
Review comment:
because TraceAnnotationMethodInterceptor and TagAnnotationMethodInterceptor also have some same methods, so move these method into TagUtil
----------------------------------------------------------------
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] lxliuxuankb commented on issue #4327: Tag annotation
supports returned expression
Posted by GitBox <gi...@apache.org>.
lxliuxuankb commented on issue #4327: Tag annotation supports returned expression
URL: https://github.com/apache/skywalking/pull/4327#issuecomment-585045240
thanks
----------------------------------------------------------------
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 #4327:
tag annotation support return expression
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4327: tag annotation support return expression
URL: https://github.com/apache/skywalking/pull/4327#discussion_r376709966
##########
File path: apm-sniffer/apm-toolkit-activation/apm-toolkit-trace-activation/src/main/java/org/apache/skywalking/apm/toolkit/activation/trace/TagAnnotationMethodInterceptor.java
##########
@@ -54,19 +54,18 @@ public void beforeMethod(
final Tags tags = method.getAnnotation(Tags.class);
if (tags != null && tags.value().length > 0) {
for (final Tag tag : tags.value()) {
- tagSpan(activeSpan, tag, context);
+ if (!TagUtil.isReturnTag(tag.value())) {
+ TagUtil.tagParamsSpan(activeSpan, context, tag.key(), tag.value());
+ }
}
}
final Tag tag = method.getAnnotation(Tag.class);
- if (tag != null) {
- tagSpan(activeSpan, tag, context);
+ if (tag != null && !TagUtil.isReturnTag(tag.value())) {
+ TagUtil.tagParamsSpan(activeSpan, context, tag.key(), tag.value());
}
}
- private void tagSpan(final AbstractSpan span, final Tag tag, final Map<String, Object> context) {
Review comment:
Why remove this method out of here?
----------------------------------------------------------------
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 #4327:
tag annotation support return expression
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4327: tag annotation support return expression
URL: https://github.com/apache/skywalking/pull/4327#discussion_r376707696
##########
File path: apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/util/TagUtil.java
##########
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.agent.core.context.util;
+
+import java.util.Map;
+
+import org.apache.skywalking.apm.agent.core.context.tag.StringTag;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.util.CustomizeExpression;
+
+/**
+ * @author: lxliuxuan Date: 2020/02/08
Review comment:
Remove author, please.
----------------------------------------------------------------
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