Add tracing logs for Nexus HTTP request retries #7186
Conversation
common/dynamicconfig/constants.go
Outdated
| NexusHTTPTraceMinAttempt = NewNamespaceIntSetting( | ||
| "system.nexusHTTPTraceMinAttempt", | ||
| 2, | ||
| `The minimum attempt of a Nexus request which will log additional HTTP request tracing information. | ||
| WARNING: setting to 0 or 1 will log additional tracing information for ALL Nexus HTTP requests and may be expensive.`, | ||
| ) | ||
| NexusHTTPTraceMaxAttempt = NewNamespaceIntSetting( | ||
| "system.nexusHTTPTraceMaxAttempt", | ||
| 5, | ||
| `The maximum attempt of a Nexus request which will log additional HTTP request tracing information. | ||
| Set to 0 to disable tracing.`, |
There was a problem hiding this comment.
This should be disabled by default. Generally, I don't think the attempt number matters here, it's more about what to trace IMHO. E.g. whether TLSHandshakeStart should be traced or not. Ideally the config would be for a set of methods, but It'd be hard to express in dynamic config as a set without adding a lot of overhead. We could probably provide a config per method we support intercepting though.
There was a problem hiding this comment.
I think attempt count is useful for limiting the overhead and the overall log spam. Without a notion of attempt count do you imagine we would log trace info for every retry?
Instead of adding a dynamic config per-hook, we could just use fx to provide the httptrace.ClientTrace. Then anyone who wants to change the behavior could do so with fx.replace
There was a problem hiding this comment.
Discussed offline, agree that adding the counts is nice, also requested that we add the dynamic config and make it global so it can be efficiently cached (a limitation of the current DC implementation).
common/nexus/trace.go
Outdated
| NewTrace(attempt int32, logger log.Logger) *httptrace.ClientTrace | ||
| } | ||
|
|
||
| var HTTPTraceConfig = dynamicconfig.NewGlobalTypedSetting( |
There was a problem hiding this comment.
Make this cached please (you'll want to wrap it with the following):
What changed?
Added additional HTTP tracing logs for the first 3 Nexus request retries.
Both minimum and maximum attempts to add additional tracing info to are configurable.
Why?
To aid in debugging.
How did you test it?
Potential risks
Documentation
Is hotfix candidate?