mrproliu commented on code in PR #104: URL: https://github.com/apache/skywalking-go/pull/104#discussion_r1359843863
########## test/plugins/scenarios/trace-activation/main.go: ########## @@ -0,0 +1,44 @@ +// 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 main + +import ( + _ "github.com/apache/skywalking-go" + "net/http" +) + +func consumerHandler(w http.ResponseWriter, r *http.Request) { + testTag() + testLog() + testGetSegmentID() + testGetSpanID() + testGetTraceID() + testSetOperationName() + testContext() + testContextCarrierAndCorrelation() +} + +func main() { + http.HandleFunc("/consumer", consumerHandler) Review Comment: Could you help to add a `/provider` handler, and test whether the correlation works or not in the different segments? ########## toolkit/trace/go.mod: ########## @@ -0,0 +1,3 @@ +module github.com/apache/skywalking-go/toolkit/trace Review Comment: I think we only need to let the `github.com/apache/skywalking-go/toolkit` as a new module, `trace` is only a part of the `toolkit`. After this, you can add the `metrics` or `logs` if you have interests. ########## plugins/core/operator/tracing.go: ########## @@ -29,4 +29,8 @@ type TracingOperator interface { CaptureContext() interface{} ContinueContext(interface{}) CleanContext() + + GetCorrelationContextValue(key string) string + SetCorrelationContextValue(key, val string) + SetCorrelationConfig(maxKeyCount, maxValueSize int) Review Comment: Config could not be set, it only read from the config file. ########## test/plugins/scenarios/trace-activation/test_service.go: ########## @@ -0,0 +1,92 @@ +// 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 main + +import ( + "github.com/apache/skywalking-go/toolkit/trace" Review Comment: looks like this file is not format, and the CI is not detected? ########## plugins/core/tracer.go: ########## @@ -65,6 +65,10 @@ func (t *Tracer) Init(entity *reporter.Entity, rep reporter.Reporter, samp Sampl t.Reporter.Boot(entity, t.cdsWatchers) t.initFlag = 1 t.initMetricsCollect(meterCollectSecond) + t.correlation = &CorrelationConfig{ Review Comment: looks like it has not been fixed? ########## toolkit/trace/api.go: ########## @@ -0,0 +1,81 @@ +// 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 trace + +func CreateEntrySpan(operationName string, extractor ExtractorRef) (s SpanRef, err error) { + return nil, err +} + +// nolint +func CreateExitSpan(operationName string, peer string, injector InjectorRef) (s SpanRef, err error) { + return nil, err +} + +func CreateLocalSpan(operationName string) (s SpanRef, err error) { + return nil, err +} + +func StopSpan() { +} + +func CaptureContext() ContextSnapshotRef { + return nil +} + +func ContinueContext(ctx ContextSnapshotRef) { +} + +func SetOperationName(string) { +} + +func GetTraceID() string { + return "" +} + +func GetSegmentID() string { + return "" +} + +func GetSpanID() int32 { + return -1 +} + +// nolint +func SetTag(key string, value string) { +} + +func SetLog(...string) { Review Comment: ```suggestion func AddLog(...string) { ``` ########## tools/go-agent/instrument/plugins/instrument.go: ########## @@ -107,8 +107,7 @@ func (i *Instrument) CouldHandle(opts *api.CompileOptions) bool { // check the version of the framework could handler version, err := i.tryToFindThePluginVersion(opts, ins) if err != nil { - logrus.Warnf("ignore the plugin %s, because: %s", ins.Name(), err) - continue + logrus.Warnf("the plugin %s %s", ins.Name(), err) Review Comment: Please give more comments and descriptions in there to explain why we can ignore it. -- 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]
