You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "Alipebt (via GitHub)" <gi...@apache.org> on 2023/10/25 06:24:30 UTC
[PR] fix: users can not use async api in toolkit-trace [skywalking-go]
Alipebt opened a new pull request, #136:
URL: https://github.com/apache/skywalking-go/pull/136
## Description
In the process of writing my blog, I found that Async API in manual APIs are not available to users because users cannot pass Span into other goroutines. I accidentally overlooked this before.
Then I referred to JAVA and found that it provides users with a `spanRef` to use async api. (I've defined this `spanRef`, but it's currently no use)
So I changed the async api.
## Change
- fix Async API in manual APIs
- Supplement cross goroutine async test
--
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
Re: [PR] fix: users can not use async api in toolkit-trace [skywalking-go]
Posted by "mrproliu (via GitHub)" <gi...@apache.org>.
mrproliu commented on code in PR #136:
URL: https://github.com/apache/skywalking-go/pull/136#discussion_r1371240884
##########
toolkit/trace/api.go:
##########
@@ -78,3 +72,13 @@ func SetCorrelation(key string, value string) {
func SetComponent(componentID int32) {
}
+
+func (*SpanRef) PrepareAsync() {
Review Comment:
Please more these methods into the `span.go`.
##########
toolkit/trace/api.go:
##########
@@ -78,3 +72,13 @@ func SetCorrelation(key string, value string) {
func SetComponent(componentID int32) {
}
+
+func (*SpanRef) PrepareAsync() {
+}
+
+func (*SpanRef) AsyncFinish() {
+}
+
+// nolint
+func (*SpanRef) Tag(key string, value string) {
Review Comment:
Please keep the same method name with the `trace.SetTag`, and the `AddLog` should be added.
##########
plugins/trace-activation/create_exitspan_intercepter.go:
##########
@@ -27,14 +27,21 @@ type CreateExitSpanInterceptor struct {
}
func (h *CreateExitSpanInterceptor) BeforeInvoke(invocation operator.Invocation) error {
- operationName := invocation.Args()[0].(string)
- peer := invocation.Args()[1].(string)
- var injector func(headerKey, headerValue string) error = invocation.Args()[2].(trace.InjectorRef)
- s, err := tracing.CreateExitSpan(operationName, peer, injector)
- invocation.DefineReturnValues(s, err)
return nil
}
func (h *CreateExitSpanInterceptor) AfterInvoke(invocation operator.Invocation, result ...interface{}) error {
+ operationName := invocation.Args()[0].(string)
+ peer := invocation.Args()[1].(string)
+ var injector func(headerKey, headerValue string) error = invocation.Args()[2].(trace.InjectorRef)
+ s, err := tracing.CreateExitSpan(operationName, peer, injector)
Review Comment:
Same here, the error message cannot be ignored.
##########
plugins/trace-activation/create_localspan_intercepter.go:
##########
@@ -26,12 +26,16 @@ type CreateLocalSpanInterceptor struct {
}
func (h *CreateLocalSpanInterceptor) BeforeInvoke(invocation operator.Invocation) error {
- operationName := invocation.Args()[0].(string)
- s, err := tracing.CreateLocalSpan(operationName)
- invocation.DefineReturnValues(s, err)
return nil
}
func (h *CreateLocalSpanInterceptor) AfterInvoke(invocation operator.Invocation, result ...interface{}) error {
+ operationName := invocation.Args()[0].(string)
+ s, _ := tracing.CreateLocalSpan(operationName)
Review Comment:
Same here, the error message cannot be ignored.
##########
toolkit/trace/span.go:
##########
@@ -21,5 +21,5 @@ type ExtractorRef func(headerKey string) (string, error)
type InjectorRef func(headerKey, headerValue string) error
Review Comment:
Can we keep all declarations in a single go file? It seems like having too many files makes it difficult for the user to locate them.
##########
plugins/trace-activation/create_entryspan_intercepter.go:
##########
@@ -27,13 +27,20 @@ type CreateEntrySpanInterceptor struct {
}
func (h *CreateEntrySpanInterceptor) BeforeInvoke(invocation operator.Invocation) error {
- operationName := invocation.Args()[0].(string)
- var extractor func(headerKey string) (string, error) = invocation.Args()[1].(trace.ExtractorRef)
- s, err := tracing.CreateEntrySpan(operationName, extractor)
- invocation.DefineReturnValues(s, err)
return nil
}
func (h *CreateEntrySpanInterceptor) AfterInvoke(invocation operator.Invocation, result ...interface{}) error {
+ operationName := invocation.Args()[0].(string)
+ var extractor func(headerKey string) (string, error) = invocation.Args()[1].(trace.ExtractorRef)
+ s, err := tracing.CreateEntrySpan(operationName, extractor)
Review Comment:
The error message cannot be ignored, we should re-write the result when the error is not nil.
--
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
Re: [PR] fix: users can not use async api in toolkit-trace [skywalking-go]
Posted by "mrproliu (via GitHub)" <gi...@apache.org>.
mrproliu commented on code in PR #136:
URL: https://github.com/apache/skywalking-go/pull/136#discussion_r1371240884
##########
toolkit/trace/api.go:
##########
@@ -78,3 +72,13 @@ func SetCorrelation(key string, value string) {
func SetComponent(componentID int32) {
}
+
+func (*SpanRef) PrepareAsync() {
Review Comment:
Please more these methods into the `span.go`.
--
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
Re: [PR] fix: users can not use async api in toolkit-trace [skywalking-go]
Posted by "mrproliu (via GitHub)" <gi...@apache.org>.
mrproliu commented on code in PR #136:
URL: https://github.com/apache/skywalking-go/pull/136#discussion_r1371323003
##########
plugins/trace-activation/create_entryspan_intercepter.go:
##########
@@ -27,13 +27,20 @@ type CreateEntrySpanInterceptor struct {
}
func (h *CreateEntrySpanInterceptor) BeforeInvoke(invocation operator.Invocation) error {
- operationName := invocation.Args()[0].(string)
- var extractor func(headerKey string) (string, error) = invocation.Args()[1].(trace.ExtractorRef)
- s, err := tracing.CreateEntrySpan(operationName, extractor)
- invocation.DefineReturnValues(s, err)
return nil
}
func (h *CreateEntrySpanInterceptor) AfterInvoke(invocation operator.Invocation, result ...interface{}) error {
+ operationName := invocation.Args()[0].(string)
+ var extractor func(headerKey string) (string, error) = invocation.Args()[1].(trace.ExtractorRef)
+ s, err := tracing.CreateEntrySpan(operationName, extractor)
+ if err != nil {
+ invocation.DefineReturnValues(nil, err)
Review Comment:
Forget `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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: users can not use async api in toolkit-trace [skywalking-go]
Posted by "mrproliu (via GitHub)" <gi...@apache.org>.
mrproliu commented on code in PR #136:
URL: https://github.com/apache/skywalking-go/pull/136#discussion_r1371323791
##########
plugins/trace-activation/create_exitspan_intercepter.go:
##########
@@ -27,14 +27,21 @@ type CreateExitSpanInterceptor struct {
}
func (h *CreateExitSpanInterceptor) BeforeInvoke(invocation operator.Invocation) error {
- operationName := invocation.Args()[0].(string)
- peer := invocation.Args()[1].(string)
- var injector func(headerKey, headerValue string) error = invocation.Args()[2].(trace.InjectorRef)
- s, err := tracing.CreateExitSpan(operationName, peer, injector)
- invocation.DefineReturnValues(s, err)
return nil
}
func (h *CreateExitSpanInterceptor) AfterInvoke(invocation operator.Invocation, result ...interface{}) error {
+ operationName := invocation.Args()[0].(string)
+ peer := invocation.Args()[1].(string)
+ var injector func(headerKey, headerValue string) error = invocation.Args()[2].(trace.InjectorRef)
+ s, err := tracing.CreateExitSpan(operationName, peer, injector)
+ if err != nil {
+ invocation.DefineReturnValues(nil, err)
Review Comment:
Forget `return`?
##########
plugins/trace-activation/create_localspan_intercepter.go:
##########
@@ -26,12 +26,19 @@ type CreateLocalSpanInterceptor struct {
}
func (h *CreateLocalSpanInterceptor) BeforeInvoke(invocation operator.Invocation) error {
- operationName := invocation.Args()[0].(string)
- s, err := tracing.CreateLocalSpan(operationName)
- invocation.DefineReturnValues(s, err)
return nil
}
func (h *CreateLocalSpanInterceptor) AfterInvoke(invocation operator.Invocation, result ...interface{}) error {
+ operationName := invocation.Args()[0].(string)
+ s, err := tracing.CreateLocalSpan(operationName)
+ if err != nil {
+ invocation.DefineReturnValues(nil, err)
Review Comment:
Forget `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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: users can not use async api in toolkit-trace [skywalking-go]
Posted by "mrproliu (via GitHub)" <gi...@apache.org>.
mrproliu merged PR #136:
URL: https://github.com/apache/skywalking-go/pull/136
--
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
Re: [PR] fix: users can not use async api in toolkit-trace [skywalking-go]
Posted by "Alipebt (via GitHub)" <gi...@apache.org>.
Alipebt commented on code in PR #136:
URL: https://github.com/apache/skywalking-go/pull/136#discussion_r1371268211
##########
toolkit/trace/span.go:
##########
@@ -21,5 +21,5 @@ type ExtractorRef func(headerKey string) (string, error)
type InjectorRef func(headerKey, headerValue string) error
Review Comment:
Ok，and how to define the file name appropriately？
##########
toolkit/trace/span.go:
##########
@@ -21,5 +21,5 @@ type ExtractorRef func(headerKey string) (string, error)
type InjectorRef func(headerKey, headerValue string) error
Review Comment:
Ok，and how to define the file name appropriately？
--
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
Re: [PR] fix: users can not use async api in toolkit-trace [skywalking-go]
Posted by "mrproliu (via GitHub)" <gi...@apache.org>.
mrproliu commented on code in PR #136:
URL: https://github.com/apache/skywalking-go/pull/136#discussion_r1371272373
##########
toolkit/trace/span.go:
##########
@@ -21,5 +21,5 @@ type ExtractorRef func(headerKey string) (string, error)
type InjectorRef func(headerKey, headerValue string) error
Review Comment:
using the `api.go` is enough.
--
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