Fix/validate compactor config

[owen-d]

Closes #2815

Tested with defined & non-defined compactor configs when:

• target=all
• target=ruler (non-compactor)
• target=compactor

[sandeepsukhani]

I was wondering if we should return a wrapped error at https://github.com/grafana/loki/pull/2824/files#diff-578848847155a57366a0b3f50e1457ff1d2aea470a8a662663aea51cb4b30ec1L49

Or add a validate function for config which checks for expected values for specific fields instead of using reflect. It would be cleaner I feel.

What do you think?

[owen-d]

I chose reflect because it's extensible (if we add a field with new defaults, etc, this code will still work). We already have a validate function for configs (I actually spent some time trying to make that work), but we unfortunately diverge from it in a few places at runtime both for validations and for module hierarchy, see

https://github.com/grafana/loki/blob/master/pkg/loki/modules.go#L419-L436
https://github.com/grafana/loki/blob/master/pkg/loki/loki.go#L361-L365

I'd definitely prefer the config validation scenario, but I wasn't able to find a clean way to refactor that. Suggestions very welcome.

As for the wrapped error, can you reformat the link? All I see is an entire commit.

[sandeepsukhani]

I chose reflect because it's extensible (if we add a field with new defaults, etc, this code will still work). We already have a validate function for configs (I actually spent some time trying to make that work), but we unfortunately diverge from it in a few places at runtime both for validations and for module hierarchy, see

https://github.com/grafana/loki/blob/master/pkg/loki/modules.go#L419-L436
https://github.com/grafana/loki/blob/master/pkg/loki/loki.go#L361-L365

I'd definitely prefer the config validation scenario, but I wasn't able to find a clean way to refactor that. Suggestions very welcome.

We could do the validation in either the factory method i.e NewCompactor or where it is being called i.e in initCompactor function in modules.

As for the wrapped error, can you reformat the link? All I see is an entire commit.
Sorry, my bad. This is where I meant returning a wrapped error https://github.com/grafana/loki/blob/d8fe7fa8577937c5a81974f163870034c131540a/pkg/storage/stores/shipper/compactor/compactor.go#L56

I am open to suggestions too and please feel free to correct me if I am wrong here.

[owen-d]

Good idea moving it into the New function @sandeepsukhani -- I've added that.

[codecov-io]

Codecov Report

Merging #2824 into master will decrease coverage by 0.08%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##           master    #2824      +/-   ##
==========================================
- Coverage   61.35%   61.26%   -0.09%     
==========================================
  Files         181      181              
  Lines       14565    14570       +5     
==========================================
- Hits         8937     8927      -10     
- Misses       4809     4824      +15     
  Partials      819      819              

Impacted Files	Coverage Δ
pkg/logql/error.go	33.33% <ø> (ø)
pkg/storage/stores/shipper/util/queries.go	82.92% <ø> (ø)
pkg/storage/stores/shipper/compactor/compactor.go	10.34% <60.00%> (+10.34%)	⬆️
pkg/promtail/positions/positions.go	46.80% <0.00%> (-11.71%)	⬇️
pkg/promtail/targets/file/tailer.go	69.52% <0.00%> (-5.72%)	⬇️
pkg/promtail/targets/file/filetarget.go	64.08% <0.00%> (-2.12%)	⬇️
pkg/logql/evaluator.go	91.64% <0.00%> (+0.40%)	⬆️
pkg/querier/queryrange/downstreamer.go	97.64% <0.00%> (+2.35%)	⬆️

[sandeepsukhani]

LGTM! Thanks for taking care of the requested changes.