panic: non-positive interval for NewTicker #2263
Comments
|
Thanks for this issue. Agree we should add this to the |
joe-elliott
added
type/bug
|
@joe-elliott Thanks for the swift response. Is the proposed workaround here adding all the fields with values to every config file? For ease of use/simplicity of config, is there a way enable the component without repeating the "defaults"? |
|
We have a series of checks that could be updated to include this trip-hazard. https://github.com/grafana/tempo/blob/main/cmd/tempo/app/config.go#L128 |
The components started in the Tempo binary are defined by the "target" and not which config blocks appear. The default target is "all" or single binary which starts all components. Just remove the empty ingester block and it will default correctly. |
|
Argh, somehow I got it in my head that having the heading enabled the component. Re-reading the docs, it's clear this is not the case - thank you for fixing my understanding. In that case, definitely agree with the Is there still be a footgun if someone tries selectively override defaults? E.g. given: ingester:
max_block_duration: 1hDoes every other field still default to the Go zero-value? |
We do what you are suggesting all the time and everything defaults as expected. So it should be fine. Here's a config we use for our docker-compose examples: I'm surprised by the behavior of just: I would have expected that to use defaults as well, but obviously it doesn't. Maybe some quirk of go yaml parsing? |
|
Possibly yes a quirk of the parser. But perhaps drop |
Confirming this is not the case. With no ingester:
...
concurrent_flushes: 4
flush_check_period: 10s
flush_op_timeout: 5m0s
trace_idle_period: 10s
max_block_duration: 30m0s
max_block_bytes: 524288000
complete_block_timeout: 15m0s
override_ring_key: ringAttempting to override one key... ingester:
max_block_duration: 1h... does not set every other field to the Go default: ingester:
...
concurrent_flushes: 4
flush_check_period: 10s
flush_op_timeout: 5m0s
trace_idle_period: 10s
max_block_duration: 1h0m0s
max_block_bytes: 524288000
complete_block_timeout: 15m0s
override_ring_key: ringSo the unintuitive behaviour happens only when |
|
I can give it a try. |
|
All that is needed here is to put a check in Config:CheckConfig that config.Ingester is not empty. That would suffice I guess. |
|
This behavior is exhibited when any struct key is present, but contains no children or value. The YAML parser treats it as a scalar value and, in the absence of a value, assigns the zero value of the type to the field. This determination is made by the lack of presence of a value or children. This applies at any depth in the parse tree, so it a much larger tripping hazard than I thought when looking at the issue the first time. I initially had the same thought as mghildiy that comparison against the zero value would be sufficient for this config section, but it applies elsewhere as well. To fully resolve this, it seems like one would have to create tests for the zero value case when a struct contains defaults. Unilaterally testing against the zero value would create a tripping hazard for new config sections that may not contain any non-zero defaults. Perhaps using struct field tags this could be denoted programmatically and validated in the config parser, but it seems like a lot of code and complexity. There is also the option of sentry values that are always assigned a non empty value and then that can be quickly tested against. Again, this seem like a potentially messy solution, but it would be effective and customizable. A quick review of the codebase suggests that there are very few/no sections without non-zero defaults, so a stop gap could be to test against the zero value for important config structs. Fixing the panic mentioned here would be as easy as adding a check to the config test that validated the duration and warned about it. For documentation it may be useful to warn users against having any empty sections if they are not explicitly trying to override default values. |
|
This issue has been automatically marked as stale because it has not had any activity in the past 60 days. |
Describe the bug
Crash on start-up when configuration heading(s?) do not contain any content. Note the new
config.verifyflag does not find any fault with the configuration.To Reproduce
Steps to reproduce the behavior:
latest(grafana/tempo@sha256:928aaab8da7320fd417ea725dcf4cbf200e7549a71c3e59245819d7469784c20) with this configuration:Expected behavior
Tempo starts with the
ingestercomponent enabled, using its default configuration, includingflush_check_period: 10sas in the manual.Actual behavior
Tempo appears to use default Go values for the
ingester.Configstruct, causingFlushCheckPeriodto default to0, causing the panic when callingtime.NewTicker().RegisterFlagsAndApplyDefaults()appears to attempt to set defaults, but these can be incorrectly overridden with the wrong thing.Environment:
The text was updated successfully, but these errors were encountered: