dynamic config: enhance update with metrics etc#9366
dynamic config: enhance update with metrics etc#9366feiyang3cat wants to merge 2 commits intotemporalio:mainfrom
Conversation
feiyang3cat
changed the title
| tag.Bool("debug-mode", debug.Enabled), | ||
| ) | ||
|
|
||
| metricsHandler, err := metrics.MetricsHandlerFromConfig(logger, cfg.Global.Metrics) |
There was a problem hiding this comment.
This is kind of unfortunate. I would rather see main() use fewer ServerOptions and rely on the defaults more, instead of the opposite. Do you think you could look into what it would take to move initialization of both metrics handler and dc client out of main and use the defaults?
There was a problem hiding this comment.
Definitely! I'd love to look into it. I will follow this in another task. so that this refinement doesn't impact a fix
There was a problem hiding this comment.
yeah I took a quick look and it's not clear to me why dynamicConfigClient need to be initialized here. The dc client config is available in temporal.NewServer as well.
Please add a TODO here.
| fc.lastUpdatedTime = modtime | ||
| defer func() { | ||
| if updateErr != nil { | ||
| fc.lastUpdatedTime = prevModtime |
There was a problem hiding this comment.
this would be cleaner if it updated forward in the defer in the no-error case
There was a problem hiding this comment.
I thought the current eager update is functioning as a kind of mutex for racers?
There was a problem hiding this comment.
I am confused, why do we need to revert the value? If a file with ModTime = X is invalid, then there's no point re-processing it periodically right, until it's updated again? lastUpdatedTime is probably not a good name, it's more like lastCheckedFileModTime
Also don't see race condition here, there's only one goroutine checking and updating this field.
feiyang3cat
force-pushed
the
dc/update-metric
branch
from
36c5a8f to
36d68a2
Compare
feiyang3cat
force-pushed
the
dc/update-metric
branch
from
36d68a2 to
eb7d16c
Compare
| tag.Bool("debug-mode", debug.Enabled), | ||
| ) | ||
|
|
||
| metricsHandler, err := metrics.MetricsHandlerFromConfig(logger, cfg.Global.Metrics) |
There was a problem hiding this comment.
yeah I took a quick look and it's not clear to me why dynamicConfigClient need to be initialized here. The dc client config is available in temporal.NewServer as well.
Please add a TODO here.
| lastUpdatedTime time.Time | ||
| config *FileBasedClientConfig | ||
| doneCh <-chan any | ||
| handler metrics.Handler |
There was a problem hiding this comment.
plz, fc.handler is a bit confusing.
| handler metrics.Handler | |
| metricsHandler metrics.Handler |
| fc.lastUpdatedTime = modtime | ||
| defer func() { | ||
| if updateErr != nil { | ||
| fc.lastUpdatedTime = prevModtime |
There was a problem hiding this comment.
I am confused, why do we need to revert the value? If a file with ModTime = X is invalid, then there's no point re-processing it periodically right, until it's updated again? lastUpdatedTime is probably not a good name, it's more like lastCheckedFileModTime
Also don't see race condition here, there's only one goroutine checking and updating this field.
4 participants
What changed?
Why?
may be a root cause why pods run into crashloop
adding this metric for alert make debugging easier
How did you test it?