[GitHub] [skywalking-go] jcchavezs commented on a diff in pull request #12: Support native http-client plugin

["jcchavezs (via GitHub)" <gi...@apache.org>]

jcchavezs commented on code in PR #12:
URL: https://github.com/apache/skywalking-go/pull/12#discussion_r1174428911


##########
plugins/http/intercepter.go:
##########
@@ -0,0 +1,66 @@
+// Licensed to 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. Apache Software Foundation (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 http
+
+import (
+	"fmt"
+	"net/http"
+
+	"github.com/apache/skywalking-go/plugins/core/operator"
+	"github.com/apache/skywalking-go/plugins/core/tracing"
+)
+
+type Interceptor struct {
+}
+
+func (h *Interceptor) BeforeInvoke(invocation *operator.Invocation) error {
+	request := invocation.Args[0].(*http.Request)
+	s, err := tracing.CreateExitSpan(fmt.Sprintf("%s:%s", request.Method, request.URL.Path), request.Host, func(headerKey, headerValue string) error {
+		request.Header.Add(headerKey, headerValue)
+		return nil
+	}, tracing.WithLayer(tracing.SpanLayerHTTP),
+		tracing.WithTag(tracing.TagHTTPMethod, request.Method),
+		tracing.WithTag(tracing.TagURL, request.Host+request.URL.Path),
+		tracing.WithComponent(5005))
+	if err != nil {
+		return err
+	}
+	invocation.Context = s
+	return nil
+}
+
+func (h *Interceptor) AfterInvoke(invocation *operator.Invocation, result ...interface{}) error {
+	if invocation.Context == nil {
+		return nil
+	}
+	span := invocation.Context.(tracing.Span)

Review Comment:
   Shall we check if span is noop to avoid function calls?



##########
tools/go-agent/tools/enhancement.go:
##########
@@ -25,10 +25,18 @@ import (
 	"github.com/dave/dst/decorator"
 )
 
+var interfaceName = "interface{}"

Review Comment:
   I think const may play better here.



##########
tools/go-agent/instrument/plugins/instrument.go:
##########
@@ -68,7 +68,7 @@ func (i *Instrument) CouldHandle(opts *api.CompileOptions) bool {
 			continue
 		}
 		// check the version of the framework could handler
-		version, err := i.tryToFindThePluginVersion(opts, ins.BasePackage())
+		version, err := i.tryToFindThePluginVersion(opts, ins)
 		if err != nil {
 			logrus.Warnf("ignore the plugin %s, because: %s", ins.Name(), err)

Review Comment:
   I would avoid this interpolation and use fields instead. Interpolation and this logging style causes unnecessary allocations.



##########
tools/go-agent/instrument/plugins/instrument.go:
##########
@@ -362,21 +362,25 @@ func (i *Instrument) validateMethodInsMatch(matcher *instrument.EnhanceMatcher,
 	return true
 }
 
-func (i *Instrument) tryToFindThePluginVersion(opts *api.CompileOptions, pkg string) (string, error) {
+func (i *Instrument) tryToFindThePluginVersion(opts *api.CompileOptions, ins instrument.Instrument) (string, error) {
 	for _, arg := range opts.AllArgs {
 		// find the go file
 		if !strings.HasSuffix(arg, ".go") {
 			continue
 		}
+		basePkg := ins.BasePackage()
 
-		parts := strings.SplitN(arg, pkg, 2)
+		parts := strings.SplitN(arg, basePkg, 2)
 		// example: github.com/gin-gonic/gin@1.1.1/gin.go
-		if len(parts) != 2 || !strings.HasPrefix(parts[1], "@") {
-			return "", fmt.Errorf("could not found the go version of the package %s, go file path: %s", pkg, arg)
+		if len(parts) != 2 {
+			return "", fmt.Errorf("could not found the go version of the package %s, go file path: %s", basePkg, arg)
+		}
+		if !strings.HasPrefix(parts[1], "@") {
+			return "", nil
 		}
 		firstDir := strings.Index(parts[1], "/")

Review Comment:
   I know it isn't part of the PR but I think strings.Cut would help.



##########
tools/go-agent/instrument/plugins/instrument.go:
##########
@@ -362,21 +362,25 @@ func (i *Instrument) validateMethodInsMatch(matcher *instrument.EnhanceMatcher,
 	return true
 }
 
-func (i *Instrument) tryToFindThePluginVersion(opts *api.CompileOptions, pkg string) (string, error) {
+func (i *Instrument) tryToFindThePluginVersion(opts *api.CompileOptions, ins instrument.Instrument) (string, error) {
 	for _, arg := range opts.AllArgs {
 		// find the go file
 		if !strings.HasSuffix(arg, ".go") {
 			continue
 		}
+		basePkg := ins.BasePackage()
 
-		parts := strings.SplitN(arg, pkg, 2)
+		parts := strings.SplitN(arg, basePkg, 2)

Review Comment:
   Better to use strings.Cut to avoid checking if the parts are actually 2.



##########
tools/go-agent/tools/types.go:
##########
@@ -0,0 +1,26 @@
+// Licensed to 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. Apache Software Foundation (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 tools
+
+// nolint
+func IsBasicDataType(name string) bool {

Review Comment:
   I wonder if using a map[string]struct{} is more performant here. Worth to add benchmarks.



-- 
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