Initial gmp exporter #342

dashpole · 2022-04-01T17:40:18Z

The new exporter adds type suffixes, and maps to the the prometheus_target monitored resource. It wraps the configuration exposed by the standard googlecloud exporter, and only exposes a subset of that configuration. The remaining fields are hard-coded to ensure a GMP-compatible export.

Customizing the reset logic will be done after #360 merges in a follow-up change.

dashpole · 2022-04-11T19:44:01Z

This is ready for review

exporter/collector/googlemanagedprometheus/factory.go

exporter/collector/googlemanagedprometheus/config.go

exporter/collector/googlemanagedprometheus_test.go

exporter/collector/googlemanagedprometheus.go

exporter/collector/config.go

dashpole · 2022-04-19T15:02:53Z

Now that the integration tests have a dependency on exporterhelper, it added some additional self-obs metrics to the integration tests.

dashpole · 2022-04-19T15:16:45Z

I confirmed that metrics from the GMP integration test are queryable via the "Managed Service for Prometheus" UI in the GCP console.

exporter/collector/googlemanagedprometheus/config.go

xichen2020 · 2022-04-25T15:15:38Z

exporter/collector/googlemanagedprometheus/factory.go

+		RetrySettings:    exporterhelper.NewDefaultRetrySettings(),
+		QueueSettings:    exporterhelper.NewDefaultQueueSettings(),
+		GMPConfig: GMPConfig{
+			UserAgent: "opentelemetry-collector-contrib {{version}}",


Note GMP convention for setting user agents: https://github.com/GoogleCloudPlatform/prometheus-engine/blob/4adcfbad6e704e02ea0f4c76e14f197fa8ec78c7/pkg/export/export.go#L217

May want to be consistent here.

+1 to a consistent layout, however also +1 to making sure we know "opentelemetry" is involved somewhere in the UA (for diagnosing bugs).

changed to opentelemetry-collector-contrib/{version} to match formatting.

Do you also want to structure it so it can be extended to include the source in the future? e.g., it may be useful to know whether the metrics are coming from Ops Agent vs otherwise.

It is settable via config options. The ops-agent could set it to: user_agent: ops-agent-gmp/{{version}} if they wanted, for example.

exporter/collector/googlemanagedprometheus/factory.go

xichen2020 · 2022-04-25T20:42:59Z

exporter/collector/googlemanagedprometheus/monitoredresource_test.go

@@ -0,0 +1,115 @@
+// Copyright 2022 Google LLC


Unit testing is useful, but it may be valuable to run GMP collector and GMP exporter side-by-side in the same testing environment and compare the exported metrics to see whether the configured resource labels for GMP exporter match expectations (and resemble the GMP collector where it makes sense)

Sounds good. We can definitely do that. If there are particular setups you would like me to test, let me know.

exporter/collector/googlemanagedprometheus/naming.go

exporter/collector/integrationtest/testdata/gmp_config.yaml

dashpole · 2022-04-28T17:33:00Z

Confirmed that summary metrics appear correctly in the Managed Prometheus promql window.

exporter/collector/googlemanagedprometheus/naming.go

xichen2020 · 2022-05-02T19:16:09Z

exporter/collector/googlemanagedprometheus/monitoredresource.go

+	return &monitoredrespb.MonitoredResource{
+		Type: "prometheus_target",
+		Labels: map[string]string{
+			"location":  getStringOrEmpty(attrs, semconv.AttributeCloudAvailabilityZone),


Thanks. I'd like to call attention to this comment block: https://github.com/GoogleCloudPlatform/prometheus-engine/blob/main/pkg/export/setup/setup.go#L77. Specifically on GKE we may not be able to always use the underlying node's zone as the location as we may have both a zonal cluster and a regional cluster in the same project with the same name, so if we use the node's zone location we'll end up creating conflicts.

xichen2020 · 2022-05-02T19:32:30Z

exporter/collector/googlemanagedprometheus/monitoredresource.go

+		Labels: map[string]string{
+			"location":  getStringOrEmpty(attrs, semconv.AttributeCloudAvailabilityZone),
+			"cluster":   getStringOrEmpty(attrs, semconv.AttributeK8SClusterName),
+			"namespace": getStringOrEmpty(attrs, semconv.AttributeK8SNamespaceName),


Thanks. If users do configure a relabel rule to set the namespace label, which takes precedence?

xichen2020 · 2022-05-02T19:41:29Z

exporter/collector/googlemanagedprometheus/monitoredresource.go

+			"cluster":   getStringOrEmpty(attrs, semconv.AttributeK8SClusterName),
+			"namespace": getStringOrEmpty(attrs, semconv.AttributeK8SNamespaceName),
+			"job":       job,
+			"instance":  getStringOrEmpty(attrs, semconv.AttributeServiceInstanceID),


I generally see the OT setup w/ Prometheus receiver and GMP exporter resembles the self-deployed GMP experience as opposed to the managed experience. If so, using whatever instance label is set by user's Prometheus configuration makes sense as is done here, but we should note this creates a different experience from managed GMP. cc/ @jsuereth

exporter/collector/googlemanagedprometheus/naming.go

dashpole · 2022-05-04T16:25:35Z

I think this should be all ready to go. @xichen2020 can you take a final pass?

xichen2020 · 2022-05-05T17:09:34Z

exporter/collector/googlemanagedprometheus/monitoredresource.go

+		job = serviceNamespace + "/" + job
+	}
+	location := getStringOrEmpty(attrs, semconv.AttributeCloudAvailabilityZone)
+	if location == "" {


Can you add a TODO and link to the issue mentioning the resource detection logic fix? Note that's also a breaking change for the exporter once merged here.

Nothing needs to be done in the exporter, which already has the correct behavior. Once the fix is in the resource detector, it will work correctly without any additional changes here.

Does that mean the GKE resource detector will use cluster-location instance attribute, determine whether the location is a zone or region (how feasible is this?), and set the zone/region attribute?

Yes, that is exactly what it will do. I have a WIP detector that does this for SDK detection if you are curious. It should be feasible to do.

I see. Would it be more robust to add a new location resource attribute and use that? That said I don't know how difficult it is to do so.

Resource attributes are defined by the otel semantic conventions: https://github.com/open-telemetry/opentelemetry-specification/blob/main/semantic_conventions/resource/cloud.yaml#L45. Right now, it defines zone + region. I suspect it would be difficult to convince them to add a third convention, "location", which is the zone or region.

I see. It seems possible to add this as a GKE-specific attribute, but maybe that's an overkill.

xichen2020 · 2022-05-05T17:19:29Z

exporter/collector/googlemanagedprometheus/monitoredresource.go

+		Labels: map[string]string{
+			"location":  getStringOrEmpty(attrs, semconv.AttributeCloudAvailabilityZone),
+			"cluster":   getStringOrEmpty(attrs, semconv.AttributeK8SClusterName),
+			"namespace": getStringOrEmpty(attrs, semconv.AttributeK8SNamespaceName),


Using target namespace is usually okay but I want to mention the kube-state-metrics use case where the namespace of the metric (not the namespace of the target) is more appropriate as the resource label. I'll leave it to you whether that's a use case the exporter should support.

xichen2020 · 2022-05-05T17:22:43Z

exporter/collector/googlemanagedprometheus/monitoredresource.go

+			"cluster":   getStringOrEmpty(attrs, semconv.AttributeK8SClusterName),
+			"namespace": getStringOrEmpty(attrs, semconv.AttributeK8SNamespaceName),
+			"job":       job,
+			"instance":  getStringOrEmpty(attrs, semconv.AttributeServiceInstanceID),


Understood. I was more saying managed GMP does it behind the scenes, whereas self-deployed GMP users would need to configure the relabeling rules themselves.

exporter/collector/metrics.go

dashpole force-pushed the initial_gmp branch 13 times, most recently from 8f649f3 to 8ee2e5a Compare April 8, 2022 12:53

dashpole force-pushed the initial_gmp branch 3 times, most recently from 89c96d9 to 7c81e04 Compare April 11, 2022 19:24

dashpole requested a review from damemi April 11, 2022 19:27

dashpole marked this pull request as ready for review April 11, 2022 19:27

dashpole requested a review from punya April 11, 2022 19:32

punya reviewed Apr 13, 2022

View reviewed changes

exporter/collector/googlemanagedprometheus/factory.go Show resolved Hide resolved

dashpole force-pushed the initial_gmp branch from 92a98fe to 7c81e04 Compare April 14, 2022 15:55

dashpole requested a review from aabmass April 15, 2022 14:56

aabmass approved these changes Apr 18, 2022

View reviewed changes

dashpole force-pushed the initial_gmp branch 2 times, most recently from 9d849bb to d856407 Compare April 19, 2022 15:01

dashpole force-pushed the initial_gmp branch 2 times, most recently from ff64376 to aa61cbd Compare April 20, 2022 20:39

xichen2020 reviewed Apr 25, 2022

View reviewed changes

dashpole force-pushed the initial_gmp branch 2 times, most recently from a2789c4 to df2c3f9 Compare April 28, 2022 17:23

xichen2020 reviewed May 2, 2022

View reviewed changes

dashpole force-pushed the initial_gmp branch 2 times, most recently from e4d07ca to 928ef75 Compare May 3, 2022 14:27

dashpole added 12 commits May 4, 2022 14:59

allow substituting resource mapping and naming functions

4ff31ee

add GMP exporter

a8bfa81

integration tests for gmp exporter

3117310

set sum of squared deviations in GMP exporter

716d5b1

perform standard normalization in GMP exporter for now

dd6565d

address feedback and add readme

f1e73e5

add summary metrics to the integration test

29cc6e5

disable normalization

c681c53

fixes after rebase

76f9db7

fall-back to region if zone is missing

b8fb88d

remove exponential histogram suffixing

184c859

return errors when naming unsupported types

c10e95e

dashpole force-pushed the initial_gmp branch from 928ef75 to c10e95e Compare May 4, 2022 15:00

gotidy

d730612

xichen2020 approved these changes May 5, 2022

View reviewed changes

address type naming nits

3b71e16

dashpole merged commit 34c09e1 into GoogleCloudPlatform:main May 5, 2022

dashpole deleted the initial_gmp branch May 5, 2022 17:57

dashpole mentioned this pull request Jul 11, 2022

Remove GMP factory functions and readme #452

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial gmp exporter #342

Initial gmp exporter #342

dashpole commented Apr 1, 2022 •

edited

dashpole commented Apr 11, 2022

dashpole commented Apr 19, 2022

dashpole commented Apr 19, 2022

xichen2020 Apr 25, 2022

jsuereth Apr 27, 2022

dashpole Apr 28, 2022

xichen2020 May 2, 2022

dashpole May 2, 2022

xichen2020 Apr 25, 2022

dashpole Apr 27, 2022

dashpole commented Apr 28, 2022

xichen2020 May 2, 2022

xichen2020 May 2, 2022

xichen2020 May 2, 2022

dashpole commented May 4, 2022

xichen2020 May 5, 2022

dashpole May 5, 2022

xichen2020 May 5, 2022

dashpole May 5, 2022

xichen2020 May 5, 2022

dashpole May 5, 2022

xichen2020 May 5, 2022

xichen2020 May 5, 2022

xichen2020 May 5, 2022

Initial gmp exporter #342

Initial gmp exporter #342

Conversation

dashpole commented Apr 1, 2022 • edited

dashpole commented Apr 11, 2022

dashpole commented Apr 19, 2022

dashpole commented Apr 19, 2022

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

dashpole commented Apr 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dashpole commented May 4, 2022

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

dashpole commented Apr 1, 2022 •

edited