Hubble: Adding structured format to configure Hubble Metrics #32294

vakalapa · 2024-05-01T22:29:41Z

Draft PR for intial changes to bring structured format to static string-based metrics enablement.

First PR in the series to implement, https://github.com/cilium/design-cfps/blob/main/cilium/CFP-30788-managing-hubble-metrics-cardinality.md

CFP: cilium/design-cfps#29

Todo:
2) Adding tests to the new structures

Please ensure your pull request adheres to the following guidelines:

For first time contributors, read Submitting a pull request
All code is covered by unit and/or runtime tests where feasible.
All commits contain a well written commit description including a title,
description and a Fixes: #XXX line if the commit addresses a particular
GitHub issue.
If your commit description contains a Fixes: <commit-id> tag, then
please add the commit author[s] as reviewer[s] to this issue.
All commits are signed off. See the section Developer’s Certificate of Origin
Provide a title or release-note blurb suitable for the release notes.
Are you a user of Cilium? Please add yourself to the Users doc
Thanks for contributing!

Fixes: #30788

<!-- Enter the release note text here if needed or remove this section! -->

maintainer-s-little-helper · 2024-05-01T22:29:44Z

Commit 714df89 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

vakalapa · 2024-05-01T22:35:12Z

pkg/hubble/metrics/api/api.go

+
+// DynamicMetricsConfig represents the structure used for configuring
+// Hubble metrics context options
+type DynamicMetricsConfig struct {


@chancez Can you help pre-review these structures before i start making more changes.

vakalapa · 2024-05-01T22:36:17Z

pkg/hubble/metrics/api/api.go

+		}
+	} else {
+		// temporarily handling comma separated values for labels context
+		for _, option := range strings.Split(s, ",") {


@chancez this is a weird case, Hubble uses two kinds of delimiters "|" for the contextOptions and "," for labelcontexts.

Yeah that's because we needed another delimiter for nested config within the already nested config.

pkg/hubble/metrics/api/options.go

Signed-off-by: vakr <vakr@microsoft.com>

anubhabMajumdar · 2024-05-02T02:38:19Z

pkg/hubble/metrics/api/api.go

+	Values ContextValues `json:"values,omitempty" yaml:"values,omitempty"`
+}
+
+type ContextOptionConfigs []*ContextOptionConfig


nit: Want to group the non-struct types together?

anubhabMajumdar · 2024-05-02T02:38:51Z

pkg/hubble/metrics/api/api.go

+
+type ContextOptionConfigs []*ContextOptionConfig
+
+type MetricConfig struct {


Exported struct needs comment for documentation?

And some comments on the individual fields too

anubhabMajumdar · 2024-05-02T02:51:24Z

pkg/hubble/metrics/api/api.go

+	Metrics []*MetricConfig `json:"metrics,omitempty" yaml:"metrics,omitempty"`
+}
+
+func (d *DynamicMetricsConfig) GetMetricNames() map[string]struct{} {


GetMetricNames -> MetricNames

Also, why return a map? The function name suggests getting a list.

anubhabMajumdar · 2024-05-02T02:57:02Z

pkg/hubble/metrics/api/api.go

+	return metrics
+}
+
+func ParseStaticMetricsConfig(enabledMetrics []string) (metricConfigs *DynamicMetricsConfig) {


nit: Can we add an example of what an enabledMetric may possibly look like?

huntergregory · 2024-05-06T02:06:56Z

pkg/hubble/metrics/api/api.go

+type ContextValues []string
+
+// ContextOptions is a structure used for configuring Hubble metrics context options
+type ContextOptionConfig struct {


Could also name this ContextOptions? Since that's the name in the ConfigMap's yaml, and it looks like there used to be a type with similar name:

Init(registry *prometheus.Registry, options Options)

huntergregory · 2024-05-06T02:14:44Z

pkg/hubble/metrics/dns/handler.go

@@ -25,15 +25,15 @@ type dnsHandler struct {
 	responseTypes *prometheus.CounterVec
 }

-func (d *dnsHandler) Init(registry *prometheus.Registry, options api.Options) error {
-	c, err := api.ParseContextOptions(options)
+func (d *dnsHandler) Init(registry *prometheus.Registry, options *api.MetricConfig) error {


nit: rename all the args that used to be options api.Options?

huntergregory · 2024-05-06T02:32:16Z

pkg/hubble/metrics/api/api.go

+
+// DynamicMetricsConfig represents the structure used for configuring
+// Hubble metrics context options
+type DynamicMetricsConfig struct {


naming idea: ConfiguredMetrics?

chancez · 2024-05-06T21:06:00Z

pkg/hubble/metrics/api/testdata/valid-metrics-config.yaml

@@ -0,0 +1,26 @@
+hubbleMetrics:


Is this file used anywhere yet?

chancez · 2024-05-06T21:10:57Z

pkg/hubble/metrics/api/api.go

@@ -146,3 +128,101 @@ var registry = NewRegistry(
 func DefaultRegistry() *Registry {
 	return registry
 }
+
+// FlowFilters is a slice of filters to include or exclude flows.
+type FlowFilters []*pb.FlowFilter


Doesn't feel necessary to introduce another type here.

chancez · 2024-05-06T21:11:38Z

pkg/hubble/metrics/api/api.go

+	Values ContextValues `json:"values,omitempty" yaml:"values,omitempty"`
+}
+
+type ContextOptionConfigs []*ContextOptionConfig


Not sure you need a type here, can just use []*ContextOptionConfig where needed.

chancez

Generally this looks pretty good to me. My only thought is we're kinda lacking on tests that actually deploy Hubble with metrics enabled so it's hard to feel confident in a bigger change like this not resulting in any regressions.

I'm not sure about the best approach to remedy that though. My current thinking is it might be wise to add enable metrics in our default helm values for CI (in .github/actions/helm-default/action.yaml or ci-required-values.yaml). That would need to be done in a separate PR though, since the changes will be ignored until they get merged into main.

github-actions · 2024-06-06T01:47:18Z

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels May 1, 2024

vakalapa force-pushed the vakalapa/structuredctxopts branch from 43aa546 to 7620ed6 Compare May 1, 2024 22:33

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 1, 2024

vakalapa commented May 1, 2024

View reviewed changes

pkg/hubble/metrics/api/options.go Outdated Show resolved Hide resolved

vakalapa force-pushed the vakalapa/structuredctxopts branch from 857b203 to 647d81c Compare May 1, 2024 23:43

Fixes:30788 feat: adding dynamic config to enable hubble metrics

decee4b

Signed-off-by: vakr <vakr@microsoft.com>

vakalapa force-pushed the vakalapa/structuredctxopts branch from 647d81c to decee4b Compare May 1, 2024 23:57

anubhabMajumdar reviewed May 2, 2024

View reviewed changes

huntergregory reviewed May 6, 2024

View reviewed changes

chancez reviewed May 6, 2024

View reviewed changes

pkg/hubble/metrics/api/testdata/valid-metrics-config.yaml

@@ -0,0 +1,26 @@

hubbleMetrics:

Copy link

Contributor

chancez May 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this file used anywhere yet?

chancez reviewed May 6, 2024

View reviewed changes

github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Jun 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hubble: Adding structured format to configure Hubble Metrics #32294

Hubble: Adding structured format to configure Hubble Metrics #32294

vakalapa commented May 1, 2024 •

edited

maintainer-s-little-helper bot commented May 1, 2024

vakalapa May 1, 2024

vakalapa May 1, 2024

chancez May 6, 2024

anubhabMajumdar May 2, 2024

anubhabMajumdar May 2, 2024

chancez May 6, 2024

anubhabMajumdar May 2, 2024

anubhabMajumdar May 2, 2024

anubhabMajumdar May 2, 2024

huntergregory May 6, 2024

huntergregory May 6, 2024

huntergregory May 6, 2024

chancez May 6, 2024

chancez May 6, 2024

chancez May 6, 2024

chancez left a comment

github-actions bot commented Jun 6, 2024


		type ContextOptionConfigs []*ContextOptionConfig

		type MetricConfig struct {

Hubble: Adding structured format to configure Hubble Metrics #32294

Are you sure you want to change the base?

Hubble: Adding structured format to configure Hubble Metrics #32294

Conversation

vakalapa commented May 1, 2024 • edited

maintainer-s-little-helper bot commented May 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chancez left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 6, 2024

vakalapa commented May 1, 2024 •

edited