You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "xzyJavaX (via GitHub)" <gi...@apache.org> on 2023/03/06 02:19:04 UTC
[GitHub] [skywalking-java] xzyJavaX opened a new pull request, #469: Override tag(AbstractTag, String) method in Entry Span
xzyJavaX opened a new pull request, #469:
URL: https://github.com/apache/skywalking-java/pull/469
<!--
⚠️ Please make sure to read this template first, pull requests that don't accord with this template
maybe closed without notice.
Texts surrounded by `<` and `>` are meant to be replaced by you, e.g. <framework name>, <issue number>.
Put an `x` in the `[ ]` to mark the item as CHECKED. `[x]`
-->
<!-- ==== 🐛 Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist 👇 ====
### Fix <bug description or the bug issue number or bug issue link>
- [ ] Add a unit test to verify that the fix works.
- [ ] Explain briefly why the bug exists and how to fix it.
==== 🐛 Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist 👆 ==== -->
<!-- ==== 🔌 Remove this line WHEN AND ONLY WHEN you're adding a new plugin, follow the checklist 👇 ====
### Add an agent plugin to support <framework name>
- [ ] Add a test case for the new plugin, refer to [the doc](https://github.com/apache/skywalking-java/blob/main/docs/en/setup/service-agent/java-agent/Plugin-test.md)
- [ ] Add a component id in [the component-libraries.yml](https://github.com/apache/skywalking/blob/master/oap-server/server-starter/src/main/resources/component-libraries.yml)
- [ ] Add a logo in [the UI repo](https://github.com/apache/skywalking-rocketbot-ui/tree/master/src/views/components/topology/assets)
==== 🔌 Remove this line WHEN AND ONLY WHEN you're adding a new plugin, follow the checklist 👆 ==== -->
<!-- ==== 📈 Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist 👇 ====
### Improve the performance of <class or module or ...>
- [ ] Add a benchmark for the improvement, refer to [the existing ones](https://github.com/apache/skywalking-java/blob/main/apm-commons/apm-datacarrier/src/test/java/org/apache/skywalking/apm/commons/datacarrier/LinkedArrayBenchmark.java)
- [ ] The benchmark result.
```text
<Paste the benchmark results here>
```
- [ ] Links/URLs to the theory proof or discussion articles/blogs. <links/URLs here>
==== 📈 Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist 👆 ==== -->
<!-- ==== 🆕 Remove this line WHEN AND ONLY WHEN you're adding a new feature, follow the checklist 👇 ====
### <Feature description>
- [ ] If this is non-trivial feature, paste the links/URLs to the design doc.
- [ ] Update the documentation to include this new feature.
- [ ] Tests(including UT, IT, E2E) are added to verify the new feature.
- [ ] If it's UI related, attach the screenshots below.
==== 🆕 Remove this line WHEN AND ONLY WHEN you're adding a new feature, follow the checklist 👆 ==== -->
- [x] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #<10480>(https://github.com/apache/skywalking/issues/10480)
- [x] Update the [`CHANGES` log](https://github.com/apache/skywalking-java/blob/main/CHANGES.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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #469: Override tag(AbstractTag, String) method in Entry Span
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #469:
URL: https://github.com/apache/skywalking-java/pull/469#discussion_r1125961492
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/EntrySpan.java:
##########
@@ -61,6 +62,14 @@ public EntrySpan tag(String key, String value) {
return this;
}
+ @Override
+ public AbstractTracingSpan tag(AbstractTag<?> tag, String value) {
+ if (stackDepth == currentMaxDepth || tag.isCanOverwrite() || isInAsyncMode) {
Review Comment:
redundant should not represent any bug. Otherwise, it is not redundant at all.
Could you explain your point?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] lujiajing1126 commented on a diff in pull request #469: Override tag(AbstractTag, String) method in Entry Span
Posted by "lujiajing1126 (via GitHub)" <gi...@apache.org>.
lujiajing1126 commented on code in PR #469:
URL: https://github.com/apache/skywalking-java/pull/469#discussion_r1125944194
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/EntrySpan.java:
##########
@@ -61,6 +62,14 @@ public EntrySpan tag(String key, String value) {
return this;
}
+ @Override
+ public AbstractTracingSpan tag(AbstractTag<?> tag, String value) {
+ if (stackDepth == currentMaxDepth || tag.isCanOverwrite() || isInAsyncMode) {
Review Comment:
But in `ExitSpan`
https://github.com/apache/skywalking-java/blob/f04866cf240ace47fa039fd5b0f548f68fb7bee6/apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/ExitSpan.java#L67-L72
Is this also redundent?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] lujiajing1126 commented on a diff in pull request #469: Override tag(AbstractTag, String) method in Entry Span
Posted by "lujiajing1126 (via GitHub)" <gi...@apache.org>.
lujiajing1126 commented on code in PR #469:
URL: https://github.com/apache/skywalking-java/pull/469#discussion_r1125944194
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/EntrySpan.java:
##########
@@ -61,6 +62,14 @@ public EntrySpan tag(String key, String value) {
return this;
}
+ @Override
+ public AbstractTracingSpan tag(AbstractTag<?> tag, String value) {
+ if (stackDepth == currentMaxDepth || tag.isCanOverwrite() || isInAsyncMode) {
Review Comment:
But in `ExitSpan`
https://github.com/apache/skywalking-java/blob/f04866cf240ace47fa039fd5b0f548f68fb7bee6/apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/ExitSpan.java#L67-L72
Is this redundant as well?
If so, we have to revert back again the expected case of `Spring 6` scenario for this PR temporally due to the issue https://github.com/apache/skywalking/issues/10479
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] lujiajing1126 commented on a diff in pull request #469: Override tag(AbstractTag, String) method in Entry Span
Posted by "lujiajing1126 (via GitHub)" <gi...@apache.org>.
lujiajing1126 commented on code in PR #469:
URL: https://github.com/apache/skywalking-java/pull/469#discussion_r1125944194
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/EntrySpan.java:
##########
@@ -61,6 +62,14 @@ public EntrySpan tag(String key, String value) {
return this;
}
+ @Override
+ public AbstractTracingSpan tag(AbstractTag<?> tag, String value) {
+ if (stackDepth == currentMaxDepth || tag.isCanOverwrite() || isInAsyncMode) {
Review Comment:
But in `ExitSpan`
https://github.com/apache/skywalking-java/blob/f04866cf240ace47fa039fd5b0f548f68fb7bee6/apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/ExitSpan.java#L67-L72
Is this redundant as well?
If so, we have to revert back again the expected case of `Spring 6` scenario due to the issue https://github.com/apache/skywalking/issues/10479
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #469: Override tag(AbstractTag, String) method in Entry Span
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #469:
URL: https://github.com/apache/skywalking-java/pull/469#issuecomment-1467534380
Make sense to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #469: Override tag(AbstractTag, String) method in Entry Span
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #469:
URL: https://github.com/apache/skywalking-java/pull/469#discussion_r1126042417
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/EntrySpan.java:
##########
@@ -61,6 +62,14 @@ public EntrySpan tag(String key, String value) {
return this;
}
+ @Override
+ public AbstractTracingSpan tag(AbstractTag<?> tag, String value) {
+ if (stackDepth == currentMaxDepth || tag.isCanOverwrite() || isInAsyncMode) {
Review Comment:
That is right. But you didn't follow what I said. I mean this logic is included when you call `super#tag`. I didn't mean this is wrong. Read `AbstractTracingSpan#tag`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] lujiajing1126 commented on a diff in pull request #469: Override tag(AbstractTag, String) method in Entry Span
Posted by "lujiajing1126 (via GitHub)" <gi...@apache.org>.
lujiajing1126 commented on code in PR #469:
URL: https://github.com/apache/skywalking-java/pull/469#discussion_r1126212781
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/EntrySpan.java:
##########
@@ -61,6 +62,14 @@ public EntrySpan tag(String key, String value) {
return this;
}
+ @Override
+ public AbstractTracingSpan tag(AbstractTag<?> tag, String value) {
+ if (stackDepth == currentMaxDepth || tag.isCanOverwrite() || isInAsyncMode) {
Review Comment:
> @lujiajing1126 BTW, why are we talking about this here? I don't think this is relative to this bug at all.
I just want to confirm the original design. But for the `ExitSpan` case, we always allow overwrite? Is that a design-oriented decision as well?
Though I suppose there are still limitations, such as the `GenericBeanFilter` case I mentioned above...
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/EntrySpan.java:
##########
@@ -61,6 +62,14 @@ public EntrySpan tag(String key, String value) {
return this;
}
+ @Override
+ public AbstractTracingSpan tag(AbstractTag<?> tag, String value) {
+ if (stackDepth == currentMaxDepth || tag.isCanOverwrite() || isInAsyncMode) {
Review Comment:
> @lujiajing1126 BTW, why are we talking about this here? I don't think this is relative to this bug at all.
I just want to confirm the original design. But for the `ExitSpan` case, we always allow overwrite? Is that a design-oriented decision as well? (since you said it is not a bug)
Though I suppose there are still limitations, such as the `GenericBeanFilter` case I mentioned above...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] kezhenxu94 commented on a diff in pull request #469: Override tag(AbstractTag, String) method in Entry Span
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on code in PR #469:
URL: https://github.com/apache/skywalking-java/pull/469#discussion_r1125855210
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/EntrySpan.java:
##########
@@ -61,6 +62,14 @@ public EntrySpan tag(String key, String value) {
return this;
}
+ @Override
+ public AbstractTracingSpan tag(AbstractTag<?> tag, String value) {
+ if (stackDepth == currentMaxDepth || tag.isCanOverwrite() || isInAsyncMode) {
Review Comment:
> > super#tag includes `isCanOverwrite` already. I think we don't need it.
> > `stackDepth == currentMaxDepth || isInAsyncMode` are the only required conditions.
>
> I suppose we still need `isCanOverwrite` check in the `EntrySpan`. `http.status_code` will hit this condition.
The parent class `AbstractTracingSpan.java#tag` already checks `isCanOverwrite` so we don't need to check again here.
https://github.com/apache/skywalking-java/blob/f04866cf240ace47fa039fd5b0f548f68fb7bee6/apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/AbstractTracingSpan.java#L127-L134
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #469: Override tag(AbstractTag, String) method in Entry Span
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #469:
URL: https://github.com/apache/skywalking-java/pull/469#discussion_r1126109916
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/EntrySpan.java:
##########
@@ -61,6 +62,14 @@ public EntrySpan tag(String key, String value) {
return this;
}
+ @Override
+ public AbstractTracingSpan tag(AbstractTag<?> tag, String value) {
+ if (stackDepth == currentMaxDepth || tag.isCanOverwrite() || isInAsyncMode) {
Review Comment:
An override tag doesn't mean you could override without depth. That is not from the design perspective.
Override tag means Spring could override Tomcat. It isn't reverted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #469: Override tag(AbstractTag, String) method in Entry Span
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #469:
URL: https://github.com/apache/skywalking-java/pull/469#issuecomment-1486244100
Close for now. No update in a month.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #469: Override tag(AbstractTag, String) method in Entry Span
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #469:
URL: https://github.com/apache/skywalking-java/pull/469#issuecomment-1467521629
Any update about 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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng closed pull request #469: Override tag(AbstractTag, String) method in Entry Span
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng closed pull request #469: Override tag(AbstractTag, String) method in Entry Span
URL: https://github.com/apache/skywalking-java/pull/469
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] lujiajing1126 commented on a diff in pull request #469: Override tag(AbstractTag, String) method in Entry Span
Posted by "lujiajing1126 (via GitHub)" <gi...@apache.org>.
lujiajing1126 commented on code in PR #469:
URL: https://github.com/apache/skywalking-java/pull/469#discussion_r1126000237
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/EntrySpan.java:
##########
@@ -61,6 +62,14 @@ public EntrySpan tag(String key, String value) {
return this;
}
+ @Override
+ public AbstractTracingSpan tag(AbstractTag<?> tag, String value) {
+ if (stackDepth == currentMaxDepth || tag.isCanOverwrite() || isInAsyncMode) {
Review Comment:
> Let's leave the bug of what it is. In this PR, we only need to fix tag set doesn't follow stack depth.
Make sense.
> redundant should not represent any bug. Otherwise, it is not redundant at all. Could you explain your point?
I do not pretty understand the original protocol. But I suppose the critical point is the priority of the `overwritable` property and the `stack depth` of the `Span`. Specifically,
1. For `EntrySpan`, does the "overwritable" tag can be overwritten even if we are not at the top of the stack?
2. For `ExitSpan`, does the "overwritable" tag can be overwritten even if we are not at the bottom of the stack?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #469: Override tag(AbstractTag, String) method in Entry Span
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #469:
URL: https://github.com/apache/skywalking-java/pull/469#discussion_r1126015159
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/EntrySpan.java:
##########
@@ -61,6 +62,14 @@ public EntrySpan tag(String key, String value) {
return this;
}
+ @Override
+ public AbstractTracingSpan tag(AbstractTag<?> tag, String value) {
+ if (stackDepth == currentMaxDepth || tag.isCanOverwrite() || isInAsyncMode) {
Review Comment:
Think in this way
About entry Span, which means an incoming RPC, we could have Tomcat -> Spring. We need the information on SpringMVC provided, that is why we asked for the deepest. Even it will have more, Tomcat -> Spring -> xxx. Then before xxx happens, Spring will override Tomcat, but eventually, xxx will override Spring.
When we move to the client side(exit span), it is reverted. Then we need the first nested span. (For example), RPC(dubbo) client -> HTTPClient -> netty, we need RPC framework's information.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] lujiajing1126 commented on a diff in pull request #469: Override tag(AbstractTag, String) method in Entry Span
Posted by "lujiajing1126 (via GitHub)" <gi...@apache.org>.
lujiajing1126 commented on code in PR #469:
URL: https://github.com/apache/skywalking-java/pull/469#discussion_r1125944194
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/EntrySpan.java:
##########
@@ -61,6 +62,14 @@ public EntrySpan tag(String key, String value) {
return this;
}
+ @Override
+ public AbstractTracingSpan tag(AbstractTag<?> tag, String value) {
+ if (stackDepth == currentMaxDepth || tag.isCanOverwrite() || isInAsyncMode) {
Review Comment:
But in `ExitSpan`
https://github.com/apache/skywalking-java/blob/f04866cf240ace47fa039fd5b0f548f68fb7bee6/apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/ExitSpan.java#L67-L72
Is this redundant as well?
If so, we have to revert back again the expected data of `Spring 6` scenario for this PR temporally due to the issue https://github.com/apache/skywalking/issues/10479
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #469: Override tag(AbstractTag, String) method in Entry Span
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #469:
URL: https://github.com/apache/skywalking-java/pull/469#discussion_r1125848600
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/EntrySpan.java:
##########
@@ -61,6 +62,14 @@ public EntrySpan tag(String key, String value) {
return this;
}
+ @Override
+ public AbstractTracingSpan tag(AbstractTag<?> tag, String value) {
+ if (stackDepth == currentMaxDepth || tag.isCanOverwrite() || isInAsyncMode) {
Review Comment:
super#tag includes `isCanOverwrite` already. I think we don't need it.
`stackDepth == currentMaxDepth || isInAsyncMode` are the only required conditions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] lujiajing1126 commented on a diff in pull request #469: Override tag(AbstractTag, String) method in Entry Span
Posted by "lujiajing1126 (via GitHub)" <gi...@apache.org>.
lujiajing1126 commented on code in PR #469:
URL: https://github.com/apache/skywalking-java/pull/469#discussion_r1126029088
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/EntrySpan.java:
##########
@@ -61,6 +62,14 @@ public EntrySpan tag(String key, String value) {
return this;
}
+ @Override
+ public AbstractTracingSpan tag(AbstractTag<?> tag, String value) {
+ if (stackDepth == currentMaxDepth || tag.isCanOverwrite() || isInAsyncMode) {
Review Comment:
> Think in this way About entry Span, which means an incoming RPC, we could have Tomcat -> Spring. We need the information on SpringMVC provided, that is why we asked for the deepest. Even it will have more, Tomcat -> Spring -> xxx. Then before xxx happens, Spring will override Tomcat, but eventually, xxx will override Spring. When we move to the client side(exit span), it is reverted. Then we need the first nested span. (For example), RPC(dubbo) client -> HTTPClient -> netty, we need RPC framework's information.
Yes. I understand how the stack works. But suppose we have an interceptor between Spring and Tomat,
```
Tomcat(before) -> Spring (before) -> [business code] -> Spring (after) -> [user-defined interceptor] -> Tomcat (after)
^
|
|
|
set http.staus_code
```
It is possible that in the `[user-defined interceptor]`, e.g. could be a [`GenericFilterBean`](https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/web/filter/GenericFilterBean.html) in this case, a new response code is set. So I guess that is where `tag.isCanOverwrite()` works.
So I think we should keep `tag.isCanOverwrite()` in both `EntrySpan` and `ExitSpan`. Then we get a chance to set a correct `http.statusCode` in the after stage of Tomcat plugin.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] lujiajing1126 commented on a diff in pull request #469: Override tag(AbstractTag, String) method in Entry Span
Posted by "lujiajing1126 (via GitHub)" <gi...@apache.org>.
lujiajing1126 commented on code in PR #469:
URL: https://github.com/apache/skywalking-java/pull/469#discussion_r1126029088
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/EntrySpan.java:
##########
@@ -61,6 +62,14 @@ public EntrySpan tag(String key, String value) {
return this;
}
+ @Override
+ public AbstractTracingSpan tag(AbstractTag<?> tag, String value) {
+ if (stackDepth == currentMaxDepth || tag.isCanOverwrite() || isInAsyncMode) {
Review Comment:
> Think in this way About entry Span, which means an incoming RPC, we could have Tomcat -> Spring. We need the information on SpringMVC provided, that is why we asked for the deepest. Even it will have more, Tomcat -> Spring -> xxx. Then before xxx happens, Spring will override Tomcat, but eventually, xxx will override Spring. When we move to the client side(exit span), it is reverted. Then we need the first nested span. (For example), RPC(dubbo) client -> HTTPClient -> netty, we need RPC framework's information.
Yes. I understand how the stack works. But suppose we have an interceptor between Spring and Tomat,
```
Tomcat(before) -> Spring (before) -> [business code] -> Spring (after) -> [user-defined interceptor] -> Tomcat (after)
^
|
|
|
set http.staus_code
```
It is possible that in the `[user-defined interceptor]`, e.g. could be a [`GenericFilterBean`](https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/web/filter/GenericFilterBean.html) in this case, a new response code is set. So I guess that is where `tag.isCanOverwrite()` works.
So I think we should keep `tag.isCanOverwrite()` in both `EntrySpan` and `ExitSpan`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #469: Override tag(AbstractTag, String) method in Entry Span
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #469:
URL: https://github.com/apache/skywalking-java/pull/469#discussion_r1126262094
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/EntrySpan.java:
##########
@@ -61,6 +62,14 @@ public EntrySpan tag(String key, String value) {
return this;
}
+ @Override
+ public AbstractTracingSpan tag(AbstractTag<?> tag, String value) {
+ if (stackDepth == currentMaxDepth || tag.isCanOverwrite() || isInAsyncMode) {
Review Comment:
> I just want to confirm the original design. But for the ExitSpan case, we always allow overwrite? Is that a design-oriented decision as well? (since you said it is not a bug)
Why always? I think I didn't say 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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] lujiajing1126 commented on a diff in pull request #469: Override tag(AbstractTag, String) method in Entry Span
Posted by "lujiajing1126 (via GitHub)" <gi...@apache.org>.
lujiajing1126 commented on code in PR #469:
URL: https://github.com/apache/skywalking-java/pull/469#discussion_r1126000237
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/EntrySpan.java:
##########
@@ -61,6 +62,14 @@ public EntrySpan tag(String key, String value) {
return this;
}
+ @Override
+ public AbstractTracingSpan tag(AbstractTag<?> tag, String value) {
+ if (stackDepth == currentMaxDepth || tag.isCanOverwrite() || isInAsyncMode) {
Review Comment:
> Let's leave the bug of what it is. In this PR, we only need to fix tag set doesn't follow stack depth.
Make sense.
> redundant should not represent any bug. Otherwise, it is not redundant at all. Could you explain your point?
I do not pretty understand the original protocol. But I suppose the critical point is the priority of the `overwritable` property and the `stack depth` of the `Span`. Specifically,
1. For `EntrySpan`, could the "overwritable" tag be overwritten even if we are not at the top of the stack?
2. For `ExitSpan`, could the "overwritable" tag be overwritten even if we are not at the bottom of the stack?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #469: Override tag(AbstractTag, String) method in Entry Span
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #469:
URL: https://github.com/apache/skywalking-java/pull/469#discussion_r1125963074
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/EntrySpan.java:
##########
@@ -61,6 +62,14 @@ public EntrySpan tag(String key, String value) {
return this;
}
+ @Override
+ public AbstractTracingSpan tag(AbstractTag<?> tag, String value) {
+ if (stackDepth == currentMaxDepth || tag.isCanOverwrite() || isInAsyncMode) {
Review Comment:
Let's leave the bug of what it is.
In this PR, we only need to fix tag set doesn't follow stack depth.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] lujiajing1126 commented on pull request #469: Override tag(AbstractTag, String) method in Entry Span
Posted by "lujiajing1126 (via GitHub)" <gi...@apache.org>.
lujiajing1126 commented on PR #469:
URL: https://github.com/apache/skywalking-java/pull/469#issuecomment-1467533622
> Any update about this?
Too many test failures. I suppose we may address https://github.com/apache/skywalking/issues/10479 first. WDYT
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] kezhenxu94 commented on a diff in pull request #469: Override tag(AbstractTag, String) method in Entry Span
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on code in PR #469:
URL: https://github.com/apache/skywalking-java/pull/469#discussion_r1125810393
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/EntrySpan.java:
##########
@@ -61,6 +62,14 @@ public EntrySpan tag(String key, String value) {
return this;
}
+ @Override
+ public AbstractTracingSpan tag(AbstractTag<?> tag, String value) {
+ if (stackDepth == currentMaxDepth || tag.isCanOverwrite() || isInAsyncMode) {
Review Comment:
Maybe 👇 ?
```suggestion
if (tag.isCanOverwrite() && (stackDepth == currentMaxDepth || isInAsyncMode)) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] lujiajing1126 commented on a diff in pull request #469: Override tag(AbstractTag, String) method in Entry Span
Posted by "lujiajing1126 (via GitHub)" <gi...@apache.org>.
lujiajing1126 commented on code in PR #469:
URL: https://github.com/apache/skywalking-java/pull/469#discussion_r1125851801
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/EntrySpan.java:
##########
@@ -61,6 +62,14 @@ public EntrySpan tag(String key, String value) {
return this;
}
+ @Override
+ public AbstractTracingSpan tag(AbstractTag<?> tag, String value) {
+ if (stackDepth == currentMaxDepth || tag.isCanOverwrite() || isInAsyncMode) {
Review Comment:
> super#tag includes `isCanOverwrite` already. I think we don't need it.
>
> `stackDepth == currentMaxDepth || isInAsyncMode` are the only required conditions.
I suppose we still need `isCanOverwrite` check in the `EntrySpan`. `http.status_code` will hit this condition.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] lujiajing1126 commented on a diff in pull request #469: Override tag(AbstractTag, String) method in Entry Span
Posted by "lujiajing1126 (via GitHub)" <gi...@apache.org>.
lujiajing1126 commented on code in PR #469:
URL: https://github.com/apache/skywalking-java/pull/469#discussion_r1126102940
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/EntrySpan.java:
##########
@@ -61,6 +62,14 @@ public EntrySpan tag(String key, String value) {
return this;
}
+ @Override
+ public AbstractTracingSpan tag(AbstractTag<?> tag, String value) {
+ if (stackDepth == currentMaxDepth || tag.isCanOverwrite() || isInAsyncMode) {
Review Comment:
> That is right. But you didn't follow what I said. I mean this logic is included when you call `super#tag`. I didn't mean this is wrong. Read `AbstractTracingSpan#tag`
I'm confused
> stackDepth == currentMaxDepth || isInAsyncMode are the only required conditions.
If this condition does not meet, how could the super method been called?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #469: Override tag(AbstractTag, String) method in Entry Span
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #469:
URL: https://github.com/apache/skywalking-java/pull/469#discussion_r1126161746
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/EntrySpan.java:
##########
@@ -61,6 +62,14 @@ public EntrySpan tag(String key, String value) {
return this;
}
+ @Override
+ public AbstractTracingSpan tag(AbstractTag<?> tag, String value) {
+ if (stackDepth == currentMaxDepth || tag.isCanOverwrite() || isInAsyncMode) {
Review Comment:
@lujiajing1126 BTW, why are we talking about this here? I don't think this is relative to this bug at all.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] lujiajing1126 commented on a diff in pull request #469: Override tag(AbstractTag, String) method in Entry Span
Posted by "lujiajing1126 (via GitHub)" <gi...@apache.org>.
lujiajing1126 commented on code in PR #469:
URL: https://github.com/apache/skywalking-java/pull/469#discussion_r1125944194
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/EntrySpan.java:
##########
@@ -61,6 +62,14 @@ public EntrySpan tag(String key, String value) {
return this;
}
+ @Override
+ public AbstractTracingSpan tag(AbstractTag<?> tag, String value) {
+ if (stackDepth == currentMaxDepth || tag.isCanOverwrite() || isInAsyncMode) {
Review Comment:
But in `ExitSpan`
https://github.com/apache/skywalking-java/blob/f04866cf240ace47fa039fd5b0f548f68fb7bee6/apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/ExitSpan.java#L67-L72
Is this redundant as well?
If so, we have to revert back again the expected case of `Spring 6` scenario for this PR due to the issue https://github.com/apache/skywalking/issues/10479
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] lujiajing1126 commented on a diff in pull request #469: Override tag(AbstractTag, String) method in Entry Span
Posted by "lujiajing1126 (via GitHub)" <gi...@apache.org>.
lujiajing1126 commented on code in PR #469:
URL: https://github.com/apache/skywalking-java/pull/469#discussion_r1125944194
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/EntrySpan.java:
##########
@@ -61,6 +62,14 @@ public EntrySpan tag(String key, String value) {
return this;
}
+ @Override
+ public AbstractTracingSpan tag(AbstractTag<?> tag, String value) {
+ if (stackDepth == currentMaxDepth || tag.isCanOverwrite() || isInAsyncMode) {
Review Comment:
But in `ExitSpan`
https://github.com/apache/skywalking-java/blob/f04866cf240ace47fa039fd5b0f548f68fb7bee6/apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/context/trace/ExitSpan.java#L67-L72
Is this also redundant?
If so, we have to revert back again the expected case of `Spring 6` scenario due to the issue https://github.com/apache/skywalking/issues/10479
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org