Optional Tracing Feature #207
Conversation
main.go
Outdated
| Propagator: propagation.TraceContext{}, | ||
| TracerProvider: trace.NewNoopTracerProvider(), | ||
| } | ||
| if v.IsSet(tracingConfigKey) { |
There was a problem hiding this comment.
Can't you just Unmarshal the candlelight.Tracing key instead of checking first?
main.go
Outdated
| Dial: (&net.Dialer{ | ||
| Timeout: t.dTimeout, | ||
| }).Dial}, | ||
| r, err := time.ParseDuration(v.GetString(reqTimeoutKey)) |
There was a problem hiding this comment.
It would be better if properties like this were part of a struct. Why not just use a TimeoutConfig struct that's part of the candlelight config?
|
@Sachin4403, just posting an update here that @johnabass is helping review this PR as well as xmidt-org/candlelight#25. |
common/utils.go
Outdated
|
|
||
|
|
||
| transactionInfoLogger, transactionLoggerOk := ctx.Value(ContextKeyTransactionInfoLogger).(kitlog.Logger) | ||
| traceId, spanId, traceOk := candlelight.ExtractTraceInformation(r.Context()) |
There was a problem hiding this comment.
This might be less noisy if candlelight.ExtractTraceInformation actually did the appending. More like:
traceLogKVs := candlelight.AppendTraceInformation(r.Context(), nil)
Then, all the logic based around traceOk can just go inside the candlelight function.
This reduces all the extra code a client has to do. If a client needs to know if tracing was appended, then returning a []interface{}, bool can address that.
There was a problem hiding this comment.
I should point out that the signature would be something like:
func AppendTraceInformation(ctx context.Context, kvs []interface{}) []interface{} /* maybe add a bool */ {
kvs = append( /* stuff */ )
return kvs
}
|
|
||
| if !ok { | ||
| errorLogger.Log(logging.MessageKey(), "transaction logger not found in context", "tid", tid,candlelight.SpanIDLogKeyName, spanId,candlelight.TraceIdLogKeyName,traceId) | ||
| if !transactionLoggerOk { |
There was a problem hiding this comment.
As a general theme, try to move this kind of conditional logic into the candlight package (see above)
joe94
force-pushed
the
feature/optionalTracing
branch
from
af52b38
to
52657af
Compare
Codecov Report
@@ Coverage Diff @@
## main #207 +/- ##
==========================================
- Coverage 76.92% 76.03% -0.90%
==========================================
Files 11 11
Lines 390 388 -2
==========================================
- Hits 300 295 -5
- Misses 84 87 +3
Partials 6 6
Continue to review full report at Codecov.
|
|
SonarCloud Quality Gate failed. |
This PR introduces: