You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "jmjoy (via GitHub)" <gi...@apache.org> on 2023/03/31 16:50:35 UTC
[GitHub] [skywalking-php] jmjoy opened a new pull request, #63: Fix parent endpoint and peer in segment ref.
jmjoy opened a new pull request, #63:
URL: https://github.com/apache/skywalking-php/pull/63
In the past, the parent endpoint and peer of the sw8 header uses the endpoint and peer of the exit span by mistake (but it does not seem to affect the display of tracing on the UI).
--
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-php] wu-sheng commented on pull request #63: Fix parent endpoint and peer in segment ref.
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #63:
URL: https://github.com/apache/skywalking-php/pull/63#issuecomment-1492844495
You could use this page to verify the result. http://demo.skywalking.apache.org/dashboard/GENERAL/Endpoint/YWdlbnQ6OnJlY29tbWVuZGF0aW9u.1/YWdlbnQ6OnJlY29tbWVuZGF0aW9u.1_L3JjbWQ=/General-Endpoint/tab/1
Of course, the e2e verification works too.
https://github.com/apache/skywalking/blob/36c76b781e0f0daacf0d72d30dc8220fce48231c/test/e2e-v2/cases/simple/simple-cases.yaml#L41-L45
--
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-php] wu-sheng commented on a diff in pull request #63: Fix parent endpoint and peer in segment ref.
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #63:
URL: https://github.com/apache/skywalking-php/pull/63#discussion_r1155063217
##########
src/plugin/plugin_curl.rs:
##########
@@ -404,8 +404,13 @@ impl CurlPlugin {
}
fn inject_sw_header(request_id: Option<i64>, ch: ZVal, info: &CurlInfo) -> crate::Result<()> {
- let sw_header = RequestContext::try_with_global_ctx(request_id, |ctx| {
- Ok(encode_propagation(ctx, info.url.path(), &info.peer))
+ let sw_header = RequestContext::try_with_global(request_id, |req_ctx| {
+ let span_object = req_ctx.entry_span.span_object();
Review Comment:
Is there any case the entry_span is null? Usually, in Java practice. we would pick the first entry span, if it can't be found, we fall back to the first span. This is usually happens this trace starts from a timer/scheduler.
--
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-php] wu-sheng commented on pull request #63: Fix parent endpoint and peer in segment ref.
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #63:
URL: https://github.com/apache/skywalking-php/pull/63#issuecomment-1492714246
It does when you check endpoint dependencies(endpoint dashboard). The graph would be totally wrong.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-php] jmjoy commented on a diff in pull request #63: Fix parent endpoint and peer in segment ref.
Posted by "jmjoy (via GitHub)" <gi...@apache.org>.
jmjoy commented on code in PR #63:
URL: https://github.com/apache/skywalking-php/pull/63#discussion_r1155111976
##########
src/plugin/plugin_curl.rs:
##########
@@ -404,8 +404,13 @@ impl CurlPlugin {
}
fn inject_sw_header(request_id: Option<i64>, ch: ZVal, info: &CurlInfo) -> crate::Result<()> {
- let sw_header = RequestContext::try_with_global_ctx(request_id, |ctx| {
- Ok(encode_propagation(ctx, info.url.path(), &info.peer))
+ let sw_header = RequestContext::try_with_global(request_id, |req_ctx| {
+ let span_object = req_ctx.entry_span.span_object();
Review Comment:
Well, I has switch to use `get_primary_span` method.
--
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-php] wu-sheng commented on a diff in pull request #63: Fix parent endpoint and peer in segment ref.
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #63:
URL: https://github.com/apache/skywalking-php/pull/63#discussion_r1155072477
##########
src/plugin/plugin_curl.rs:
##########
@@ -404,8 +404,13 @@ impl CurlPlugin {
}
fn inject_sw_header(request_id: Option<i64>, ch: ZVal, info: &CurlInfo) -> crate::Result<()> {
- let sw_header = RequestContext::try_with_global_ctx(request_id, |ctx| {
- Ok(encode_propagation(ctx, info.url.path(), &info.peer))
+ let sw_header = RequestContext::try_with_global(request_id, |req_ctx| {
+ let span_object = req_ctx.entry_span.span_object();
Review Comment:
I just suggest maybe it is better to add a method to retrieve the endpoint(looking for entrySpan's) rather than hard-coding this everywhere.
--
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-php] wu-sheng merged pull request #63: Fix parent endpoint and peer in segment ref and tag url in entry span.
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng merged PR #63:
URL: https://github.com/apache/skywalking-php/pull/63
--
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-php] jmjoy commented on a diff in pull request #63: Fix parent endpoint and peer in segment ref.
Posted by "jmjoy (via GitHub)" <gi...@apache.org>.
jmjoy commented on code in PR #63:
URL: https://github.com/apache/skywalking-php/pull/63#discussion_r1155065260
##########
src/plugin/plugin_curl.rs:
##########
@@ -404,8 +404,13 @@ impl CurlPlugin {
}
fn inject_sw_header(request_id: Option<i64>, ch: ZVal, info: &CurlInfo) -> crate::Result<()> {
- let sw_header = RequestContext::try_with_global_ctx(request_id, |ctx| {
- Ok(encode_propagation(ctx, info.url.path(), &info.peer))
+ let sw_header = RequestContext::try_with_global(request_id, |req_ctx| {
+ let span_object = req_ctx.entry_span.span_object();
Review Comment:
Now PHP trace always start from HTTP requests, so the entry span will exist, I think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-php] jmjoy commented on pull request #63: Fix parent endpoint and peer in segment ref.
Posted by "jmjoy (via GitHub)" <gi...@apache.org>.
jmjoy commented on PR #63:
URL: https://github.com/apache/skywalking-php/pull/63#issuecomment-1492849640
> You could use this page to verify the result. http://demo.skywalking.apache.org/dashboard/GENERAL/Endpoint/YWdlbnQ6OnJlY29tbWVuZGF0aW9u.1/YWdlbnQ6OnJlY29tbWVuZGF0aW9u.1_L3JjbWQ=/General-Endpoint/tab/1
>
> Of course, the e2e verification works too. https://github.com/apache/skywalking/blob/36c76b781e0f0daacf0d72d30dc8220fce48231c/test/e2e-v2/cases/simple/simple-cases.yaml#L41-L45
How I test it in my personal UI, and it works.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-php] wu-sheng commented on a diff in pull request #63: Fix parent endpoint and peer in segment ref.
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #63:
URL: https://github.com/apache/skywalking-php/pull/63#discussion_r1155063304
##########
src/plugin/plugin_curl.rs:
##########
@@ -404,8 +404,13 @@ impl CurlPlugin {
}
fn inject_sw_header(request_id: Option<i64>, ch: ZVal, info: &CurlInfo) -> crate::Result<()> {
- let sw_header = RequestContext::try_with_global_ctx(request_id, |ctx| {
- Ok(encode_propagation(ctx, info.url.path(), &info.peer))
+ let sw_header = RequestContext::try_with_global(request_id, |req_ctx| {
+ let span_object = req_ctx.entry_span.span_object();
Review Comment:
Or sometimes, the plugin doesn't cover the entry. So, only the current exit span with some local spans left.
--
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