-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Doc: regenerate API refs (FlowCollector + Flow Format) #550
Conversation
FYI @skrthomas |
to see the new Flow Format spec in github's renderer: https://github.com/netobserv/network-observability-operator/blob/a5606eef057a8e40c798d1cc2609cc8998468ef0/docs/flows-format.adoc |
docs/fields.yaml
Outdated
@@ -0,0 +1,129 @@ | |||
# Fields definition, used to generate documentation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add this to the static-frontend-config ?
I would like to get rid of ipfix.ts
file from console plugin repo in the end.
We can also rely on these definitions to check if field is a label and add types since the query engine check for numbers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can surely move that to the config, ok, but it will stay unused in code for the time being, right? at least for this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well then I can also change my flow-format-validation
script into a go test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, that's a long run refactor 😉
3473a2e
to
946b71d
Compare
|
||
The "Filter ID" column shows which related name to use when defining Quick Filters (see `spec.consolePlugin.quickFilters` in the `FlowCollector` specification). | ||
|
||
The "Loki label" column is useful when querying Loki directly: label fields need to be selected using link:https://grafana.com/docs/loki/latest/logql/log_queries/#log-stream-selector[stream selectors]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need to write this too:
If you are reading this specification as a reference for the Kafka export feature, you must treat all Labels and Fields as regular fields and ignore any distinctions between them that are specific to Loki.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I removed that because now it's not organized by Fields and Labels anymore - it's directly written for Kafka exporter users
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Joel. I re-copied your latest IBMSG tweaks to my PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-copied and pushed latest to my PR.
// Flaky assertion here, quite annoying, some weirdness with `envtest` maybe? | ||
// Anyway it doesn't bring much value. Disabling it for the time being |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this is not tied to the parallelization of the tests. Currently, we are not setting any parameter wich means tests from different packages can run concurently.
-parallel n
Allow parallel execution of test functions that call t.Parallel, and
fuzz targets that call t.Parallel when running the seed corpus.
The value of this flag is the maximum number of tests to run
simultaneously.
While fuzzing, the value of this flag is the maximum number of
subprocesses that may call the fuzz function simultaneously, regardless of
whether T.Parallel is called.
By default, -parallel is set to the value of GOMAXPROCS.
Setting -parallel to values higher than GOMAXPROCS may cause degraded
performance due to CPU contention, especially when fuzzing.
Note that -parallel only applies within a single test binary.
The 'go test' command may run tests for different packages
in parallel as well, according to the setting of the -p flag
(see 'go help build').
Adding -p 1
to the go test
command ensure each tests to be run separately but highly increase the testing time.
We could also try to use the t.Parallel
call in each test checking for Cleanup
Lastly, we can also force different FlowCollector instance name. It's hacky but may work here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI this PR moved FLP and Monitoring controller tests outside of controllers
package so we parallelize creation / deletion of FlowCollector
between the 3 packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, after many tests, I didn't find issues with parallelism (although I don't guarantee that there aren't any). My guess is that it's ok with parallelism because each controller set up its own envtest environment so they run in isolation. And tbh if this isn't the case, I'm quite surprised to not see many more failures at random places, whereas here this is always the same single test that is failing.
Anyway my understanding of this test failure is finally that this is a timing issue, the CR taking sometimes more time to be actually deleted than the test timeout. So I have this PR for a fix: #557 (and I'll remove my change here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted the change here, cf the mentioned PR above
- name: PktDropLatestState | ||
type: string | ||
description: TCP state on last dropped packet | ||
filter: pkt_drop_state # couldn't guess from config | ||
- name: PktDropLatestDropCause | ||
type: string | ||
description: Latest drop cause | ||
filter: pkt_drop_cause # couldn't guess from config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- name: PktDropLatestState | |
type: string | |
description: TCP state on last dropped packet | |
filter: pkt_drop_state # couldn't guess from config | |
- name: PktDropLatestDropCause | |
type: string | |
description: Latest drop cause | |
filter: pkt_drop_cause # couldn't guess from config | |
- name: PktDropLatestState | |
type: string | |
description: TCP state on last dropped packet | |
- name: PktDropLatestDropCause | |
type: string | |
description: Latest drop cause |
We keep filter definition in the columns right ?
type FieldConfig struct { | ||
Name string `yaml:"name" json:"name"` | ||
Type string `yaml:"type" json:"type"` | ||
Description string `yaml:"description" json:"description"` | ||
LokiLabel bool `yaml:"lokiLabel,omitempty" json:"lokiLabel,omitempty"` | ||
Filter string `yaml:"filter,omitempty" json:"filter,omitempty"` | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type FieldConfig struct { | |
Name string `yaml:"name" json:"name"` | |
Type string `yaml:"type" json:"type"` | |
Description string `yaml:"description" json:"description"` | |
LokiLabel bool `yaml:"lokiLabel,omitempty" json:"lokiLabel,omitempty"` | |
Filter string `yaml:"filter,omitempty" json:"filter,omitempty"` | |
} | |
type FieldConfig struct { | |
Name string `yaml:"name" json:"name"` | |
Type string `yaml:"type" json:"type"` | |
Description string `yaml:"description" json:"description"` | |
LokiLabel bool `yaml:"lokiLabel,omitempty" json:"lokiLabel,omitempty"` | |
} |
I don't think we should define filters in the field configuration. Filters are already defined in their own section and tied to columns when needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they aren't defined from there, it's only a reference. In fact, for almost all the fields I didn't need that reference as I could retrieve them from the columns. But there are two exceptions: for drop state and drop cause. These ones don't have the filter defined in column, I guess they must be some kind of special cases in the front implementation? Because of them, as I want to document the existence of these filters, I had to add an explicit reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can rename the field filterRef
to clarify
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are multiple exceptions indeed such as:
TimeFlowStartMs
/TimeFlowEndMs
PktDropLatestState
/PktDropLatestDropCause
Duplicate
So either we refactor these in the plugin to rely on filters or we force these filter ids from the field config as you initially planned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see filters for TimeFlowStartMs
/ TimeFlowEndMs
or Duplicate
, while they exist for PktDrop stuff (in the filters
yaml)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a refactoring will be necessary moving forward, but for this PR I didn't intend to do any code change, just extracting whatever data I can for the doc
@jotak: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #550 +/- ##
==========================================
+ Coverage 57.87% 57.89% +0.02%
==========================================
Files 73 73
Lines 9543 9543
==========================================
+ Hits 5523 5525 +2
Misses 3684 3684
+ Partials 336 334 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Flow format is now generated from a fields.yaml file stored here, instead of the console plugin's type definition previously used, as it became more likely to be outdated A check is added to make sure any field referenced in the frontend config is also defined in fields.yaml
Removed hack/flows-format-validation.sh, replaced with a go test Add type information
- for instance => for example - may => might Also use head directive ":_mod-docs-content-type: REFERENCE"
Co-authored-by: Julien Pinsonneau <[email protected]>
Co-authored-by: Julien Pinsonneau <[email protected]>
@jpinsonneau PR updated (doc regenerated after your updates, reverted flaky test change) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good for the doc, thanks @jotak !
thanks |
[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 |
* Doc: regenerate API refs (FlowCollector + Flow Format) Flow format is now generated from a fields.yaml file stored here, instead of the console plugin's type definition previously used, as it became more likely to be outdated A check is added to make sure any field referenced in the frontend config is also defined in fields.yaml * fix validation script * Merge fields.yaml in static-frontend-config.yaml Removed hack/flows-format-validation.sh, replaced with a go test Add type information * Minor tweaks for IBM style guide - for instance => for example - may => might Also use head directive ":_mod-docs-content-type: REFERENCE" * Update controllers/consoleplugin/config/static-frontend-config.yaml Co-authored-by: Julien Pinsonneau <[email protected]> * Update hack/asciidoc-flows-gen.sh Co-authored-by: Julien Pinsonneau <[email protected]> * Revert changes on flaky test * regen doc --------- Co-authored-by: Julien Pinsonneau <[email protected]>
* Doc: regenerate API refs (FlowCollector + Flow Format) Flow format is now generated from a fields.yaml file stored here, instead of the console plugin's type definition previously used, as it became more likely to be outdated A check is added to make sure any field referenced in the frontend config is also defined in fields.yaml * fix validation script * Merge fields.yaml in static-frontend-config.yaml Removed hack/flows-format-validation.sh, replaced with a go test Add type information * Minor tweaks for IBM style guide - for instance => for example - may => might Also use head directive ":_mod-docs-content-type: REFERENCE" * Update controllers/consoleplugin/config/static-frontend-config.yaml Co-authored-by: Julien Pinsonneau <[email protected]> * Update hack/asciidoc-flows-gen.sh Co-authored-by: Julien Pinsonneau <[email protected]> * Revert changes on flaky test * regen doc --------- Co-authored-by: Julien Pinsonneau <[email protected]>
* Doc: regenerate API refs (FlowCollector + Flow Format) Flow format is now generated from a fields.yaml file stored here, instead of the console plugin's type definition previously used, as it became more likely to be outdated A check is added to make sure any field referenced in the frontend config is also defined in fields.yaml * fix validation script * Merge fields.yaml in static-frontend-config.yaml Removed hack/flows-format-validation.sh, replaced with a go test Add type information * Minor tweaks for IBM style guide - for instance => for example - may => might Also use head directive ":_mod-docs-content-type: REFERENCE" * Update controllers/consoleplugin/config/static-frontend-config.yaml * Update hack/asciidoc-flows-gen.sh * Revert changes on flaky test * regen doc --------- Co-authored-by: Julien Pinsonneau <[email protected]>
Flow format is now generated from a fields.yaml file stored here, instead of the console plugin's type definition previously used, as it became more likely to be outdated
A check is added to make sure any field referenced in the frontend config is also defined in fields.yaml