mrproliu commented on code in PR #209: URL: https://github.com/apache/skywalking-go/pull/209#discussion_r1802543034
########## plugins/gozero/go.mod: ########## @@ -0,0 +1,11 @@ +module github.com/apache/skywalking-go/plugins/gozero + +go 1.19 + +require github.com/apache/skywalking-go/plugins/core v0.0.0-20241010090250-3981a26a05db Review Comment: we are already using `go.work` to management all dependencies, so I think this require is not necessary. ########## plugins/gozero/instrument.go: ########## @@ -0,0 +1,135 @@ +// 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 gozero + +import ( + "embed" + "github.com/apache/skywalking-go/plugins/core/instrument" + "strings" + + _ "fmt" Review Comment: Why need to import here? ########## plugins/gozero/zrpc/server_middleware_intercepter.go: ########## @@ -0,0 +1,56 @@ +package zrpc + +import ( + "context" + "github.com/apache/skywalking-go/plugins/core/operator" + "github.com/apache/skywalking-go/plugins/core/tracing" + "github.com/zeromicro/go-zero/zrpc" + "google.golang.org/grpc" +) + +type ServerMiddlewareInterceptor struct { +} + +// BeforeInvoke intercepts the HTTP request before invoking the handler. +func (h *ServerMiddlewareInterceptor) BeforeInvoke(invocation operator.Invocation) error { + server := invocation.CallerInstance().(*zrpc.RpcServer) + server.AddUnaryInterceptors(RpcServeInterceptor(invocation)) + return nil +} + +// AfterInvoke processes after the HTTP request has been handled. +func (h *ServerMiddlewareInterceptor) AfterInvoke(invocation operator.Invocation, result ...interface{}) error { + return nil +} + +// RpcServeInterceptor is a grpc server interceptor that creates a new span for each incoming request. +var RpcServeInterceptor = func(invocation operator.Invocation) grpc.UnaryServerInterceptor { + return func(ctx context.Context, req any, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (resp any, err error) { + s, err := tracing.CreateLocalSpan(info.FullMethod, + tracing.WithLayer(tracing.SpanLayerRPCFramework), + tracing.WithTag(tracing.TagURL, info.FullMethod), + tracing.WithComponent(5023), + ) + if err != nil { + return nil, err + } + defer s.End() + + resp, err = handler(ctx, req) + if err != nil { + s.Error(err.Error()) + } + // collect request and response data + if config.CollectRequestParameters { + //TODO: collect request data Review Comment: Please fix TODO. ########## plugins/gozero/zrpc/server_middleware_intercepter.go: ########## @@ -0,0 +1,56 @@ +package zrpc + +import ( + "context" + "github.com/apache/skywalking-go/plugins/core/operator" + "github.com/apache/skywalking-go/plugins/core/tracing" + "github.com/zeromicro/go-zero/zrpc" + "google.golang.org/grpc" +) + +type ServerMiddlewareInterceptor struct { +} + +// BeforeInvoke intercepts the HTTP request before invoking the handler. +func (h *ServerMiddlewareInterceptor) BeforeInvoke(invocation operator.Invocation) error { + server := invocation.CallerInstance().(*zrpc.RpcServer) + server.AddUnaryInterceptors(RpcServeInterceptor(invocation)) + return nil +} + +// AfterInvoke processes after the HTTP request has been handled. +func (h *ServerMiddlewareInterceptor) AfterInvoke(invocation operator.Invocation, result ...interface{}) error { + return nil +} + +// RpcServeInterceptor is a grpc server interceptor that creates a new span for each incoming request. +var RpcServeInterceptor = func(invocation operator.Invocation) grpc.UnaryServerInterceptor { + return func(ctx context.Context, req any, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (resp any, err error) { + s, err := tracing.CreateLocalSpan(info.FullMethod, + tracing.WithLayer(tracing.SpanLayerRPCFramework), + tracing.WithTag(tracing.TagURL, info.FullMethod), + tracing.WithComponent(5023), + ) + if err != nil { + return nil, err + } + defer s.End() + + resp, err = handler(ctx, req) + if err != nil { + s.Error(err.Error()) + } + // collect request and response data + if config.CollectRequestParameters { + //TODO: collect request data + //s.Tag(tracing.TagRpcRequest, "request") + } + // collect response data + if config.CollectResponseData { + //TODO: collect response data Review Comment: Please fix TODO. ########## test/plugins/scenarios/gozero/plugin.yml: ########## @@ -0,0 +1,25 @@ +# 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. + +entry-service: http://${HTTP_HOST}:${HTTP_PORT}/user/info Review Comment: I can't see the excepted file to validate all generated tracing data? ########## test/plugins/scenarios/gozero/README.md: ########## @@ -0,0 +1,6 @@ + +### 生成rpc代码 Review Comment: Why user need this documentation? If you want to running this command before starting testing, please add this into [startup.sh](https://github.com/apache/skywalking-go/pull/209/files#diff-a57cf9e1481207842ec8431d1a36ccfc1823f0e77f2aadfe9b9af5d6b8e6790a) ########## plugins/gozero/rest/config.go: ########## @@ -0,0 +1,9 @@ +package rest + +//skywalking:config gozero +var config struct { + CollectRequestParameters bool `config:"collect_request_parameters"` // CollectRequestParameters is used to determine whether to collect request parameters. + CollectResponseData bool `config:"collect_response_data"` // CollectResponseData is used to determine whether to collect response data. + DataSensitiveFields string `config:"data_sensitive_fields"` // DataSensitiveFields is used to determine the sensitive fields in the request and response data. Review Comment: I can't see any place have use this configuration. ########## test/plugins/run.sh: ########## @@ -204,7 +204,7 @@ for framework_version in $frameworks; do -go-agent ${go_agent} > ${case_logs}/runner-helper.log echo "staring the testcase ${scenario_name}, ${case_name}" - + dos2unix ${case_home}/scenarios.sh Review Comment: Please remove unnecessary codes. ########## plugins/gozero/rest/server_middleware_intercepter.go: ########## @@ -0,0 +1,69 @@ +package rest + +import ( + "fmt" + "github.com/apache/skywalking-go/plugins/core/operator" + "github.com/apache/skywalking-go/plugins/core/tracing" + "github.com/zeromicro/go-zero/rest" + "net/http" +) + +type ServerMiddlewareInterceptor struct { +} + +// skyWalking middleware +var SkyWalkingMiddleware rest.Middleware = func(next http.HandlerFunc) http.HandlerFunc { + return func(writer http.ResponseWriter, request *http.Request) { + s, err := tracing.CreateEntrySpan(fmt.Sprintf("%s:%s", request.Method, request.URL.Path), func(headerKey string) (string, error) { + return request.Header.Get(headerKey), nil + }, tracing.WithLayer(tracing.SpanLayerHTTP), + tracing.WithTag(tracing.TagHTTPMethod, request.Method), + tracing.WithTag(tracing.TagURL, request.Host+request.URL.Path), + tracing.WithComponent(5023)) + if err != nil { + next(writer, request) + return + } + + defer s.End() + + // collect response data + if config.CollectRequestParameters { + switch request.Method { + case http.MethodGet: + if request.URL.RawQuery != "" { + s.Tag(tracing.TagHTTPParams, request.URL.RawQuery) + } + case http.MethodPost, http.MethodPut, http.MethodPatch: + //TODO collect request body data + if request.Body != nil { + } + } + } + + // collect response data + if config.CollectResponseData { + //TODO: collect response data Review Comment: Please fix the TODO. ########## test/plugins/Makefile: ########## @@ -18,12 +18,16 @@ GO = go GO_BUILD = $(GO) build +REPODIR := $(shell dirname $(realpath $(firstword $(MAKEFILE_LIST))))/../../ +VERSION_PATH = $(REPODIR)/VERSION +VERSION ?= $(shell grep version $(VERSION_PATH) | awk -F ': ' '{print $$2}') + .PHONY: build build: mkdir -p dist # go agent @make -C ../../tools/go-agent linux - cp ../../bin/skywalking-go-agent--linux-$(shell go env GOARCH) dist/skywalking-go-agent + cp ../../bin/skywalking-go-agent-$(VERSION)-linux-$(shell go env GOARCH) dist/skywalking-go-agent Review Comment: Please do not submit content unrelated to this plugin. If necessary, please open a separate PR, and we can discuss it there. ########## test/plugins/scenarios/gozero/pb/userpb/user_grpc.testpb.go: ########## @@ -0,0 +1,163 @@ +// Code generated by protoc-gen-go-grpc. DO NOT EDIT. Review Comment: I think these files can be generated by the plugin test, right? ########## test/plugins/scenarios/gozero/handler.go: ########## @@ -0,0 +1,101 @@ +package main + +import ( + "github.com/zeromicro/go-zero/rest" + "github.com/zeromicro/go-zero/rest/httpx" + "net/http" +) + +func RegisterApiHandlers(server *rest.Server, serverCtx *ApiServiceContext) { + server.AddRoutes( + []rest.Route{ + { + Method: http.MethodGet, + Path: "/health", + Handler: func(w http.ResponseWriter, r *http.Request) { + httpx.OkJson(w, "ok") + }, + }, + }, + ) + server.AddRoutes( + []rest.Route{ + { + // 用户查找 Review Comment: Please remove all Chinese comments. ########## test/plugins/scenarios/gozero/bin/startup.sh: ########## @@ -0,0 +1,22 @@ +#!/bin/bash +# +# 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. + +home="$(cd "$(dirname $0)"; pwd)" +go build ${GO_BUILD_OPTS} -o gozero + +./gozero Review Comment: Please activate all plugin configuration before start, we should check all features are works or not. ########## plugins/gozero/rest/server_middleware_intercepter.go: ########## @@ -0,0 +1,69 @@ +package rest + +import ( + "fmt" + "github.com/apache/skywalking-go/plugins/core/operator" + "github.com/apache/skywalking-go/plugins/core/tracing" + "github.com/zeromicro/go-zero/rest" + "net/http" +) + +type ServerMiddlewareInterceptor struct { +} + +// skyWalking middleware +var SkyWalkingMiddleware rest.Middleware = func(next http.HandlerFunc) http.HandlerFunc { + return func(writer http.ResponseWriter, request *http.Request) { + s, err := tracing.CreateEntrySpan(fmt.Sprintf("%s:%s", request.Method, request.URL.Path), func(headerKey string) (string, error) { + return request.Header.Get(headerKey), nil + }, tracing.WithLayer(tracing.SpanLayerHTTP), + tracing.WithTag(tracing.TagHTTPMethod, request.Method), + tracing.WithTag(tracing.TagURL, request.Host+request.URL.Path), + tracing.WithComponent(5023)) + if err != nil { + next(writer, request) + return + } + + defer s.End() + + // collect response data + if config.CollectRequestParameters { + switch request.Method { + case http.MethodGet: + if request.URL.RawQuery != "" { + s.Tag(tracing.TagHTTPParams, request.URL.RawQuery) + } + case http.MethodPost, http.MethodPut, http.MethodPatch: + //TODO collect request body data + if request.Body != nil { + } + } + } + + // collect response data + if config.CollectResponseData { + //TODO: collect response data + //s.Tag(tracing.TagHTTPResponse, "resp") + } + + // set trace id to response header + if config.OutputTraceId { + writer.Header().Set("trace-id", s.TraceID()) Review Comment: Can we validate whether the trace ID is setting success or not in the plugin 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
