Copilot commented on code in PR #1030: URL: https://github.com/apache/dubbo-go-samples/pull/1030#discussion_r2785373425
########## logger/trace-integration/cmd/main.go: ########## @@ -0,0 +1,81 @@ +/* + * 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. + */ + +package main + +import ( + "context" + "time" + + "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/sdk/trace" + + "dubbo.apache.org/dubbo-go/v3" + _ "dubbo.apache.org/dubbo-go/v3/imports" + log "dubbo.apache.org/dubbo-go/v3/logger" + "dubbo.apache.org/dubbo-go/v3/protocol" + + "github.com/dubbogo/gost/log/logger" +) + +func main() { + ins, err := dubbo.NewInstance( + dubbo.WithProtocol( + protocol.WithTriple(), + protocol.WithPort(20000), + ), + dubbo.WithLogger( + log.WithZap(), + log.WithLevel("debug"), + log.WithTraceIntegration(true), + log.WithRecordErrorToSpan(true), + ), + ) + if err != nil { + panic(err) + } + + server, err := ins.NewServer() + if err != nil { + panic(err) + } + go server.Serve() + + // setup OpenTelemetry tracer + tp := trace.NewTracerProvider() + otel.SetTracerProvider(tp) + tracer := tp.Tracer("demo") + + traceCtx, span := tracer.Start(context.Background(), "demo-operation") + defer span.End() + rawLogger := logger.GetLogger() + ctxLog := rawLogger.(log.CtxLogger) + ctx, cancel := context.WithTimeout(context.Background(), time.Second*3) + defer cancel() + for { + select { + case <-ctx.Done(): + return + default: + ctxLog.CtxInfo(traceCtx, "hello dubbogo this is info log") + ctxLog.CtxDebug(traceCtx, "hello dubbogo this is debug log") + ctxLog.CtxWarn(traceCtx, "hello dubbogo this is warn log") + ctxLog.CtxError(traceCtx, "hello dubbogo this is error log") Review Comment: `ctx` is only used as a timeout signal, but the log calls use `traceCtx` (a different context). This makes the sample harder to follow and may result in logs not carrying the same cancellation/deadline semantics as the controlling context. Consider deriving the timeout context from `traceCtx` (e.g., `context.WithTimeout(traceCtx, ...)`) and using that same context for `CtxInfo/CtxDebug/...` calls, or remove the extra context and use a timer for the loop exit. ```suggestion timeoutCtx, cancel := context.WithTimeout(traceCtx, time.Second*3) defer cancel() for { select { case <-timeoutCtx.Done(): return default: ctxLog.CtxInfo(timeoutCtx, "hello dubbogo this is info log") ctxLog.CtxDebug(timeoutCtx, "hello dubbogo this is debug log") ctxLog.CtxWarn(timeoutCtx, "hello dubbogo this is warn log") ctxLog.CtxError(timeoutCtx, "hello dubbogo this is error log") ``` ########## logger/README.md: ########## @@ -74,3 +78,51 @@ Then call SetLogger method to set logger ```go logger.SetLogger(&customLogger{}) ``` + +#### Integrate OpenTelemetry trace information + +logger supports OpenTelemetry trace integration, automatically injecting trace information into logs. + +##### Method 1: New API (Recommended) + +```go +ins, err := dubbo.NewInstance( + dubbo.WithProtocol( + protocol.WithTriple(), + protocol.WithPort(20000), + ), + dubbo.WithLogger( + log.WithZap(), + log.WithTraceIntegration(true), + log.WithRecordErrorToSpan(true), + ), +) +``` + +##### Method 2: Old API + +```go +loggerConfig := config.NewLoggerConfigBuilder(). + SetDriver("zap"). + SetTraceIntegrationEnabled(true). + SetRecordErrorToSpan(true). + Build() +loggerConfig.Init() +``` + +##### Using CtxLogger + +```go +rawLogger := gostLogger.GetLogger() +ctxLog := rawLogger.(logger.CtxLogger) + +ctxLog.CtxInfo(ctx, "hello dubbogo this is info log") +ctxLog.CtxDebug(ctx, "hello dubbogo this is debug log") +ctxLog.CtxWarn(ctx, "hello dubbogo this is warn log") +ctxLog.CtxError(ctx, "hello dubbogo this is error log") + +ctxLog.CtxInfof(ctx, "user: %s", "alice") +ctxLog.CtxDebugf(ctx, "value: %d", 42) +ctxLog.CtxWarnf(ctx, "latency: %dms", 150) +ctxLog.CtxErrorf(ctx, "failed: %v", err) Review Comment: The CtxLogger usage snippet refers to `gostLogger.GetLogger()` and `logger.CtxLogger`, but neither `gostLogger` nor `ctx` are introduced in the snippet, and it’s ambiguous which `logger` package defines `CtxLogger`. This makes the example hard to copy/paste. Consider updating the snippet to use the same identifiers as the sample code (e.g., `rawLogger := logger.GetLogger()` from `github.com/dubbogo/gost/log/logger`, assert to `dubbo-go/v3/logger.CtxLogger`), and show how `ctx` is obtained (e.g., from `tracer.Start(...)` or an incoming request context). ```suggestion import ( "context" dubboLogger "github.com/apache/dubbo-go/v3/logger" gostLogger "github.com/dubbogo/gost/log/logger" ) func ExampleCtxLogger() { // In real applications, ctx usually comes from tracer.Start(...) or an incoming request. ctx := context.Background() rawLogger := gostLogger.GetLogger() ctxLog, ok := rawLogger.(dubboLogger.CtxLogger) if !ok { // underlying logger does not implement CtxLogger return } ctxLog.CtxInfo(ctx, "hello dubbogo this is info log") ctxLog.CtxDebug(ctx, "hello dubbogo this is debug log") ctxLog.CtxWarn(ctx, "hello dubbogo this is warn log") ctxLog.CtxError(ctx, "hello dubbogo this is error log") ctxLog.CtxInfof(ctx, "user: %s", "alice") ctxLog.CtxDebugf(ctx, "value: %d", 42) ctxLog.CtxWarnf(ctx, "latency: %dms", 150) ctxLog.CtxErrorf(ctx, "failed: %v", someErr) } ``` ########## logger/README_zh.md: ########## @@ -68,4 +72,52 @@ type Logger interface { 然后调用 SetLogger 方法设置 logger ```go logger.SetLogger(&customLogger{}) -``` \ No newline at end of file +``` + +#### 集成 OpenTelemetry trace 信息 + +logger 支持 OpenTelemetry trace 集成，自动将 trace 信息注入日志。 + +##### 方式1：New API（推荐） + +```go +ins, err := dubbo.NewInstance( + dubbo.WithProtocol( + protocol.WithTriple(), + protocol.WithPort(20000), + ), + dubbo.WithLogger( + log.WithZap(), + log.WithTraceIntegration(true), + log.WithRecordErrorToSpan(true), + ), +) +``` + +##### 方式2：Old API + +```go +loggerConfig := config.NewLoggerConfigBuilder(). + SetDriver("zap"). + SetTraceIntegrationEnabled(true). + SetRecordErrorToSpan(true). + Build() +loggerConfig.Init() +``` + +##### 使用 CtxLogger 记录日志 + +```go +rawLogger := gostLogger.GetLogger() +ctxLog := rawLogger.(logger.CtxLogger) + +ctxLog.CtxInfo(ctx, "hello dubbogo this is info log") +ctxLog.CtxDebug(ctx, "hello dubbogo this is debug log") +ctxLog.CtxWarn(ctx, "hello dubbogo this is warn log") +ctxLog.CtxError(ctx, "hello dubbogo this is error log") + +ctxLog.CtxInfof(ctx, "user: %s", "alice") +ctxLog.CtxDebugf(ctx, "value: %d", 42) +ctxLog.CtxWarnf(ctx, "latency: %dms", 150) +ctxLog.CtxErrorf(ctx, "failed: %v", err) Review Comment: `CtxLogger` 的示例代码中使用了 `gostLogger.GetLogger()` 和 `logger.CtxLogger`，但片段里没有说明 `gostLogger` / `ctx` 的来源，也不清楚 `logger` 指的是哪个包，导致示例不易直接复制使用。建议与 sample 代码保持一致：使用 `rawLogger := logger.GetLogger()`（`github.com/dubbogo/gost/log/logger`），断言为 `dubbo-go/v3/logger.CtxLogger`，并补充 `ctx` 的获取方式（例如 `tracer.Start(...)` 返回的 context 或请求的 context）。 ```suggestion import ( "context" dubboLogger "github.com/apache/dubbo-go/v3/logger" gostLogger "github.com/dubbogo/gost/log/logger" ) // ctx 通常来自 tracer.Start(...) 返回的 context，或者请求的 context（如 HTTP/gRPC 请求） func foo(ctx context.Context, err error) { rawLogger := gostLogger.GetLogger() ctxLog, ok := rawLogger.(dubboLogger.CtxLogger) if !ok { // 当前 logger 未实现 CtxLogger 接口，这里根据需要进行降级处理 return } ctxLog.CtxInfo(ctx, "hello dubbogo this is info log") ctxLog.CtxDebug(ctx, "hello dubbogo this is debug log") ctxLog.CtxWarn(ctx, "hello dubbogo this is warn log") ctxLog.CtxError(ctx, "hello dubbogo this is error log") ctxLog.CtxInfof(ctx, "user: %s", "alice") ctxLog.CtxDebugf(ctx, "value: %d", 42) ctxLog.CtxWarnf(ctx, "latency: %dms", 150) ctxLog.CtxErrorf(ctx, "failed: %v", err) } ``` ########## logger/trace-integration/cmd/main.go: ########## @@ -0,0 +1,81 @@ +/* + * 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. + */ + +package main + +import ( + "context" + "time" + + "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/sdk/trace" + + "dubbo.apache.org/dubbo-go/v3" + _ "dubbo.apache.org/dubbo-go/v3/imports" + log "dubbo.apache.org/dubbo-go/v3/logger" + "dubbo.apache.org/dubbo-go/v3/protocol" + + "github.com/dubbogo/gost/log/logger" +) + +func main() { + ins, err := dubbo.NewInstance( + dubbo.WithProtocol( + protocol.WithTriple(), + protocol.WithPort(20000), + ), + dubbo.WithLogger( + log.WithZap(), + log.WithLevel("debug"), + log.WithTraceIntegration(true), + log.WithRecordErrorToSpan(true), + ), + ) + if err != nil { + panic(err) + } + + server, err := ins.NewServer() + if err != nil { + panic(err) + } + go server.Serve() + + // setup OpenTelemetry tracer + tp := trace.NewTracerProvider() + otel.SetTracerProvider(tp) Review Comment: The `trace.TracerProvider` is created and set globally, but it is never shut down. In OpenTelemetry SDKs, `TracerProvider.Shutdown` should be called to flush and release resources (even in samples) and to avoid goroutine/resource leaks when the program exits. Consider adding a deferred `tp.Shutdown(...)` with a bounded timeout context. ```suggestion otel.SetTracerProvider(tp) shutdownCtx, shutdownCancel := context.WithTimeout(context.Background(), 5*time.Second) defer func() { defer shutdownCancel() if err := tp.Shutdown(shutdownCtx); err != nil { log.Errorf("failed to shutdown tracer provider: %v", err) } }() ``` -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
