AlexStocks commented on code in PR #3195: URL: https://github.com/apache/dubbo-go/pull/3195#discussion_r2969137099
########## logger/core/zap/ctx_logger.go: ########## @@ -0,0 +1,161 @@ +/* + * 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 zap + +import ( + "context" + "errors" + "fmt" +) + +import ( + dubbogoLogger "github.com/dubbogo/gost/log/logger" + + "go.opentelemetry.io/otel/codes" + + "go.opentelemetry.io/otel/trace" + + "go.uber.org/zap" +) + +import ( + "dubbo.apache.org/dubbo-go/v3/logger" +) + +// ZapCtxLogger wraps DubboLogger with context-aware logging support. +type ZapCtxLogger struct { + *dubbogoLogger.DubboLogger + recordErrorToSpan bool +} + +var _ logger.CtxLogger = (*ZapCtxLogger)(nil) + +func NewZapCtxLogger(base *dubbogoLogger.DubboLogger, recordErrorToSpan bool) *ZapCtxLogger { + return &ZapCtxLogger{ + DubboLogger: base, + recordErrorToSpan: recordErrorToSpan, + } +} + +// withTraceFields injects trace information from context into logger. +func (l *ZapCtxLogger) withTraceFields(ctx context.Context) *zap.SugaredLogger { + zapLogger, ok := l.Logger.(*zap.SugaredLogger) + if !ok { + return nil + } Review Comment: withTraceFields 里 type assertion `l.Logger.(*zap.SugaredLogger)` 如果 ok=false，返回 nil，调用方 CtxDebugf 等方法会 fallback 到非 context-aware 的日志。如果 trace integration 开启了但 logger 底层不是预期的具体类型，会静默降级，调试困难。 建议：在 instantiate 里如果 traceEnabled=true 但类型不符合预期，打一条 warn log 说明原因，方便排查。 ########## logger/base.go: ########## @@ -41,3 +45,16 @@ type Logger interface { Fatal(args ...any) Fatalf(fmt string, args ...any) } + Review Comment: CtxLogger 接口继承了 Logger，但 DubboLogger 本身不实现 CtxLogger，所以 logger.Logger 类型无法在编译期保证实现了 CtxLogger。用户拿到 Logger 接口后想用 CtxInfo 需要 type assertion，不够直观。 建议：在 Logger 接口里加一个类型方法或者内嵌 CtxLogger，让用户可以统一用 interface assertion： ```go var l logger.Logger = GetLogger() if ctxLogger, ok := l.(logger.CtxLogger); ok { ctxLogger.CtxInfo(ctx, "test") } ``` 目前用 ok 判断可以工作，但注释里应该说明这个用法。 ########## logger/trace_extractor.go: ########## @@ -0,0 +1,60 @@ +/* + * 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 logger + +import ( + "context" +) + +import ( + "go.opentelemetry.io/otel/trace" +) + +// TraceFields contains trace information extracted from context. +type TraceFields struct { + TraceID string + SpanID string + TraceFlags string +} + +// ExtractTraceFields extracts trace information from the given context. +func ExtractTraceFields(ctx context.Context) TraceFields { Review Comment: context.TODO() 返回的是非 nil 的 context，但 spanCtx 必定 invalid。这种情况下 HasTrace 返回 false 是对的，但 ExtractTraceFields 返回全零值，调用方无法区分是真的没有 trace 还是只是用了 TODO context。 建议：给 TraceFields 加一个 Valid bool 字段，或者在调用方判断 TraceID != "" 而不是直接信任返回值。 -- 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]
