Skip to content
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

include a flag to disable metric prefixes #2198

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions metrics/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (

"go.opencensus.io/stats"
corev1 "k8s.io/api/core/v1"
"knative.dev/pkg/configmap"
"knative.dev/pkg/metrics/metricskey"
)

Expand All @@ -43,9 +44,11 @@ const (
// The following keys are used to configure metrics reporting.
// See https://github.com/knative/serving/blob/main/config/config-observability.yaml
// for details.
collectorAddressKey = "metrics.opencensus-address"
collectorSecureKey = "metrics.opencensus-require-tls"
reportingPeriodKey = "metrics.reporting-period-seconds"
collectorAddressKey = "metrics.opencensus-address"
collectorSecureKey = "metrics.opencensus-require-tls"
reportingPeriodKey = "metrics.reporting-period-seconds"
enableDeprecatedMetricPrefixKey = "metrics.enable-deprecated-name-prefix"
requestMetricsBackendKey = "metrics.request-metrics-backend-destination"

defaultBackendEnvName = "DEFAULT_METRICS_BACKEND"
defaultPrometheusPort = 9090
Expand Down Expand Up @@ -84,6 +87,9 @@ type metricsConfig struct {
// If duration is less than or equal to zero, it enables the default behavior.
reportingPeriod time.Duration

// whether or not we prefix the metric name with the domain and/or component
enableDeprecatedMetricPrefix bool

// recorder provides a hook for performing custom transformations before
// writing the metrics to the stats.RecordWithOptions interface.
recorder func(context.Context, []stats.Measurement, ...stats.Options) error
Expand Down Expand Up @@ -220,6 +226,14 @@ func createMetricsConfig(_ context.Context, ops ExporterOptions) (*metricsConfig
mc.reportingPeriod = 5 * time.Second
}
}

err := configmap.Parse(m,
configmap.AsBool(enableDeprecatedMetricPrefixKey, &mc.enableDeprecatedMetricPrefix))

if err != nil {
return nil, fmt.Errorf("failed to parse %s: %w", enableDeprecatedMetricPrefixKey, err)
}

return &mc, nil
}

Expand Down
22 changes: 16 additions & 6 deletions metrics/config_observability.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,13 +79,22 @@ type ObservabilityConfig struct {
// MetricsCollectorAddress specifies the metrics collector address. This is only used
// when the metrics backend is opencensus.
MetricsCollectorAddress string

// EnableDeprecatedMetricPrefix retains the legacy behaviour of prefixing our metric names
// with either the component name and/or metric domain
//
// When enabled:
// - OpenCensus exporter prefixes the domain and component
// - Prometheus exporter prefixes the component name (as a prom namespace)
EnableDeprecatedMetricPrefix bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a breaking change and I see there is a plan here for mirgation, next question would be if there is a plan for removing this flag or users should just keep this flag on.

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably worth surfacing this topic on the thread.

}

func defaultConfig() *ObservabilityConfig {
return &ObservabilityConfig{
LoggingURLTemplate: DefaultLogURLTemplate,
RequestLogTemplate: DefaultRequestLogTemplate,
RequestMetricsBackend: defaultRequestMetricsBackend,
LoggingURLTemplate: DefaultLogURLTemplate,
RequestLogTemplate: DefaultRequestLogTemplate,
RequestMetricsBackend: defaultRequestMetricsBackend,
EnableDeprecatedMetricPrefix: true,
}
}

Expand All @@ -97,11 +106,12 @@ func NewObservabilityConfigFromConfigMap(configMap *corev1.ConfigMap) (*Observab
cm.AsBool("logging.enable-var-log-collection", &oc.EnableVarLogCollection),
cm.AsString("logging.revision-url-template", &oc.LoggingURLTemplate),
cm.AsString(ReqLogTemplateKey, &oc.RequestLogTemplate),
cm.AsBool("profiling.enable", &oc.EnableProfiling),
cm.AsBool(EnableReqLogKey, &oc.EnableRequestLog),
cm.AsBool(EnableProbeReqLogKey, &oc.EnableProbeRequestLog),
cm.AsString("metrics.request-metrics-backend-destination", &oc.RequestMetricsBackend),
cm.AsBool("profiling.enable", &oc.EnableProfiling),
cm.AsString("metrics.opencensus-address", &oc.MetricsCollectorAddress),
cm.AsString(requestMetricsBackendKey, &oc.RequestMetricsBackend),
cm.AsString(collectorAddressKey, &oc.MetricsCollectorAddress),
cm.AsBool(enableDeprecatedMetricPrefixKey, &oc.EnableDeprecatedMetricPrefix),
); err != nil {
return nil, err
}
Expand Down
30 changes: 16 additions & 14 deletions metrics/config_observability_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,22 +35,24 @@ func TestObservabilityConfiguration(t *testing.T) {
}{{
name: "observability configuration with all inputs",
wantConfig: &ObservabilityConfig{
EnableProbeRequestLog: true,
EnableProfiling: true,
EnableVarLogCollection: true,
EnableRequestLog: true,
LoggingURLTemplate: "https://logging.io",
RequestLogTemplate: `{"requestMethod": "{{.Request.Method}}"}`,
RequestMetricsBackend: "opencensus",
EnableProbeRequestLog: true,
EnableProfiling: true,
EnableVarLogCollection: true,
EnableRequestLog: true,
EnableDeprecatedMetricPrefix: false,
LoggingURLTemplate: "https://logging.io",
RequestLogTemplate: `{"requestMethod": "{{.Request.Method}}"}`,
RequestMetricsBackend: "opencensus",
},
data: map[string]string{
EnableProbeReqLogKey: "true",
"logging.enable-var-log-collection": "true",
ReqLogTemplateKey: `{"requestMethod": "{{.Request.Method}}"}`,
"logging.revision-url-template": "https://logging.io",
EnableReqLogKey: "true",
"metrics.request-metrics-backend-destination": "opencensus",
"profiling.enable": "true",
EnableProbeReqLogKey: "true",
"logging.enable-var-log-collection": "true",
ReqLogTemplateKey: `{"requestMethod": "{{.Request.Method}}"}`,
"logging.revision-url-template": "https://logging.io",
EnableReqLogKey: "true",
"profiling.enable": "true",
requestMetricsBackendKey: "opencensus",
enableDeprecatedMetricPrefixKey: "false",
},
}, {
name: "observability config with no map",
Expand Down
7 changes: 4 additions & 3 deletions metrics/e2e_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,10 @@ func TestMetricsExport(t *testing.T) {
Component: testComponent,
PrometheusPort: prometheusPort,
ConfigMap: map[string]string{
BackendDestinationKey: string(backend),
collectorAddressKey: ocFake.address,
reportingPeriodKey: "1",
BackendDestinationKey: string(backend),
collectorAddressKey: ocFake.address,
reportingPeriodKey: "1",
enableDeprecatedMetricPrefixKey: "true",
},
}
}
Expand Down
2 changes: 1 addition & 1 deletion metrics/opencensus_exporter.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ func newOpenCensusExporter(config *metricsConfig, logger *zap.SugaredLogger) (vi
opts = append(opts, ocagent.WithAddress(config.collectorAddress))
}
metrixPrefix := path.Join(config.domain, config.component)
if metrixPrefix != "" {
if metrixPrefix != "" && config.enableDeprecatedMetricPrefix {
opts = append(opts, ocagent.WithMetricNamePrefix(metrixPrefix))
}
if config.requireSecure {
Expand Down
6 changes: 5 additions & 1 deletion metrics/prometheus_exporter.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,11 @@ func (emptyPromExporter) ExportView(viewData *view.Data) {

//nolint: unparam // False positive of flagging the second result of this function unused.
func newPrometheusExporter(config *metricsConfig, logger *zap.SugaredLogger) (view.Exporter, ResourceExporterFactory, error) {
e, err := prom.NewExporter(prom.Options{Namespace: config.component})
opts := prom.Options{}
if config.enableDeprecatedMetricPrefix {
opts.Namespace = config.component
Copy link
Contributor

@skonto skonto Jul 26, 2021

Choose a reason for hiding this comment

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

Why we set the Namespace to be equal to a component name here (just curious)?

Copy link
Member Author

Choose a reason for hiding this comment

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

this is the legacy behaviour - which I want to preserve with this flag

}
e, err := prom.NewExporter(opts)
if err != nil {
logger.Errorw("Failed to create the Prometheus exporter.", zap.Error(err))
return nil, nil, err
Expand Down
11 changes: 6 additions & 5 deletions metrics/prometheus_exporter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,12 @@ func TestNewPrometheusExporter(t *testing.T) {
}{{
name: "port 9090",
config: metricsConfig{
domain: "does not matter",
component: testComponent,
backendDestination: prometheus,
prometheusPort: 9090,
prometheusHost: "0.0.0.0",
domain: "does not matter",
component: testComponent,
backendDestination: prometheus,
enableDeprecatedMetricPrefix: true,
prometheusPort: 9090,
prometheusHost: "0.0.0.0",
},
expectedAddr: "0.0.0.0:9090",
}, {
Expand Down