Use refactored confgen for metrics #142
Conversation
|
Skipping CI for Draft Pull Request. |
jotak
force-pushed
the
use-refactored-confgen
branch
from
8a15c87 to
b0e5bb4
Compare
jotak
force-pushed
the
use-refactored-confgen
branch
from
b0e5bb4 to
d97d013
Compare
| func TestMergeMetricsConfigurationNoIgnore(t *testing.T) { | ||
| assert := assert.New(t) | ||
|
|
||
| flp := getFLPConfig() | ||
| FlpMetricsConfigDir = "test_metrics_definitions" | ||
| FlpMetricsConfig = TestFlpMetricsConfig | ||
|
|
There was a problem hiding this comment.
The idea here was to use a well-known (unchanging) set of metrics for testing, and not the current version in metrics_definitions, which might change from time-to-time. If we use the regular metrics_definitions, they whenever we make changes to metrics_definitions we may also have to update the test.
There was a problem hiding this comment.
Ok, but on the other hand, there's also some value in testing the actual embedded metrics (making sure it's correctly formed, etc.). Could be a bit more maintenance , but it also increases coverage
jotak
force-pushed
the
use-refactored-confgen
branch
from
d97d013 to
0243e3f
Compare
jotak
force-pushed
the
use-refactored-confgen
branch
from
0243e3f to
3805c93
Compare
- Simplified interface with confgen: just call "ParseDefinition" and "GenerateTruncatedConfig" - Propagate errors to caller
jotak
force-pushed
the
use-refactored-confgen
branch
from
3805c93 to
1168cec
Compare
| srcPath := FlpMetricsConfigDir + "/" + fileName | ||
| destPath := tmpMetricsDefinitionsDir + "/" + fileName | ||
| input, err := FlpMetricsConfig.ReadFile(srcPath) | ||
| srcPath := metricsConfigDir + "/" + fileName |
There was a problem hiding this comment.
Could filepath.Join() be used to build srcPath?
https://pkg.go.dev/path/filepath#Join
| config := map[string]interface{}{ | ||
| "log-level": b.desired.LogLevel, |
There was a problem hiding this comment.
config.ConfigFileStruct can be used instead of map[string]interface{}
There was a problem hiding this comment.
hmm there are some weird things happening here: ConfigFileStruct doesn't have health. Seems like the output of confgen isn't exactly the input of flp ?
There was a problem hiding this comment.
also LogLevel exists in ConfigFileStruct but it seems unused (see config.ParseConfig => it doesn't fill log-level).
I guess some more cleanup should be done on FLP
There was a problem hiding this comment.
Looking at the code of FLP's main, it seems to me that FLP accepts the health port only from the command line arguments.
https://github.com/netobserv/flowlogs-pipeline/blob/d7656a24bb2417bfd2fec25f75fb1ee077c1b86e/cmd/flowlogs-pipeline/main.go
If I'm correct, I think it's a bug in FLP that it doesn't allow the health port to be set from the config file.
There was a problem hiding this comment.
I think it works, as it uses viper which accepts a config file as a substitute to command line args.
But viper does use any of the declared model that we have, so that's why it is a little bit confusing IMO
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jotak The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Requires: netobserv/flowlogs-pipeline#267
Opened as draft until the above is merged
cc @KalmanMeth there's a couple of little changes on top of what you added :