-
Notifications
You must be signed in to change notification settings - Fork 24
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
NETOBSERV-1610: document cardinality warning per field #623
Conversation
@jotak: This pull request references NETOBSERV-1610 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
@jotak: This pull request references NETOBSERV-1610 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
@OlivierCazade I guess this could also be leveraged in https://issues.redhat.com/browse/NETOBSERV-1447 ? @skrthomas : in openshift/openshift-docs#74412 : I guess in "Configuring custom metrics" we should explain to refer to this fields list to check what is fine to use (mentioned as "fine" in fields list), or to use with care (mentioned as "careful"), or that cannot be used (mentioned as "avoid") - those mentions will now appear in the Flows format doc. I've updated my other PR with more info on cardinality: fe3a9e6 |
docs/flows-format.adoc
Outdated
@@ -9,105 +9,132 @@ The "Filter ID" column shows which related name to use when defining Quick Filte | |||
|
|||
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]. | |||
|
|||
The "Cardinality" column gives information about the implied metric cardinality if this field was to be used as a Prometheus label with the `FlowMetrics` API. Refer to the `FlowMetrics` documentation for more information on using this API. |
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.
cc @skrthomas for reviewing this part
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.
@jotak Linking to the flows format and mentioning the cardinality in the text sounds good. Thanks for looping me in here so I can see this update now.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #623 +/- ##
=======================================
Coverage 66.42% 66.42%
=======================================
Files 65 65
Lines 7357 7357
=======================================
Hits 4887 4887
Misses 2114 2114
Partials 356 356
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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, thanks @jotak
Co-authored-by: Sara Thomas <sarthoma@redhat.com>
New changes are detected. LGTM label has been removed. |
/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 |
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.
@OlivierCazade where should these warnings appear? I created flowmetric resource with DstAddr
label but did not see any warning when creating the resource.
Description
cardinalityWarn
property to frontend fields configDependencies
n/a
Checklist
If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.