fix issue with shim use of config.Duration #7996
Conversation
| @@ -887,7 +887,7 @@ func (e *Endpoint) chunkify(ctx context.Context, res *resourceKind, now time.Tim | |||
| // Determine time of last successful collection | |||
| metricName := e.getMetricNameForId(metric.CounterId) | |||
| if metricName == "" { | |||
| e.log.Info("Unable to find metric name for id %d. Skipping!", metric.CounterId) | |||
| e.log.Infof("Unable to find metric name for id %d. Skipping!", metric.CounterId) | |||
There was a problem hiding this comment.
This is a good change but looks unrelated. Was it supposed to be in this pr?
There was a problem hiding this comment.
Unrelated; just don't want to make a new pr for one line. :D
| @@ -54,3 +54,24 @@ func TestLoadingConfig(t *testing.T) { | |||
|
|
|||
| require.Len(t, c.Processors, 1) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
This test belongs in config/config_test.go instead of here. TestConfig_LoadSpecialTypes is similar.
There was a problem hiding this comment.
you can't have config require reverse_dns due to cyclical dependencies. Since it mentions both, it has to go here.
There was a problem hiding this comment.
.. though, a config_test package might work.
There was a problem hiding this comment.
Looking at this with fresh eyes, the test above it is almost identical in setup. I'm not so sure that it belongs in config. The functionality in config is covered by another test (in config_test above), and this was just an extra sanity test I added to make sure the plugin had what it needed, and wasn't falling back on default values. In the end this was not needed (because it's not a regression test per se, and didn't highlight the bug I was fixing), but I figured it didn't hurt to add the test to the test suite. I'd recommend leaving it as is.
There was a problem hiding this comment.
I understand it's convenient to put it here because of dependencies, but it doesn't belong here because it's testing config parsing, not reverse dns functionality. I'll leave it to you to move if you want.
| @@ -54,3 +54,24 @@ func TestLoadingConfig(t *testing.T) { | |||
|
|
|||
| require.Len(t, c.Processors, 1) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
I understand it's convenient to put it here because of dependencies, but it doesn't belong here because it's testing config parsing, not reverse dns functionality. I'll leave it to you to move if you want.
The execd shim loads config in a slightly different way. As a result, the config.Duration wasn't being handled properly.
This PR fixes it, and resolves issues mentioned in #7911