[receiver/prometheusremotewrite] Parse labels #35656

ArthurSens · 2024-10-07T18:34:04Z

Description

This PR builds on top of #35535, #35565 and #35624.

Here we're parsing labels into resource/metric attributes. It's still not great because resource attributes (with exception to service.namespace, service.name and service.name.id) are encoded into a special metric called target_info. Metrics related to specific target infos may arrive in separate write requests, so it may be impossible to build the full OTLP metric in a stateless way.

In this PR I'm ignoring this problem 😛, and transforming job and instance labels into resource attributes, while all other labels become scope attributes.

Please focus on the latest commit when reviewing this PR :)
1c9ff80

dashpole · 2024-10-09T18:17:34Z

I don't think we should be putting metric labels into the resource attributes.

ArthurSens · 2024-10-13T21:34:13Z

I don't think we should be putting metric labels into the resource attributes.

I was in doubt about this one. Since there are three layers of attributes, how do we know where labels should go when transformed into OpenTelemetry metrics?

dashpole · 2024-10-14T13:24:11Z

I was in doubt about this one. Since there are three layers of attributes, how do we know where labels should go when transformed into OpenTelemetry metrics?

Job + Info become service.* labels. Other labels on prom metrics become labels on the otel metric. Labels on target_info become attributes on the resource. The tricky part is that target_info might end up in a different request? We can deal with that in a follow-up

ArthurSens · 2024-10-17T19:42:52Z

The tricky part is that target_info might end up in a different request? We can deal with that in a follow-up

Oof, I honestly haven't thought about that. This makes this receiver a lot more difficult to write 😬

ArthurSens · 2024-10-30T15:38:44Z

Alright, PR updated to only handle job and instance as resource attributes. All the rest becomes scope attributes (I couldn't find attributes in the metrics object 🤔).

receiver/prometheusremotewritereceiver/receiver.go

dashpole · 2024-10-31T15:14:52Z

receiver/prometheusremotewritereceiver/receiver.go

+
+			scopeAttributes := rm.ScopeMetrics().AppendEmpty().Scope().Attributes()
+			for _, l := range ls {
+				if l.Name == "instance" || l.Name == "job" || l.Name == labels.MetricName {


We should also omit otel_scope_name and otel_scope_version, which should be used to set the name and version of the scope (if present).

ArthurSens · 2024-11-04T21:17:24Z

Sorry for the massive review request folks, accidentally messed up my commits 😭

…c attributes Signed-off-by: Arthur Silva Sens <[email protected]>

dashpole · 2024-11-11T21:11:32Z

.chloggen/prwreceiver-parselabels.yaml

+# Include 'user' if the change is relevant to end users.
+# Include 'api' if there is a change to a library API.
+# Default: '[user]'
+change_logs: [api, user]


I don't see any changes to the public API of the package.

Suggested change

change_logs: [api, user]

change_logs: []

dashpole · 2024-11-11T21:12:31Z

receiver/prometheusremotewritereceiver/receiver.go

-	return pmetric.NewMetrics(), promremote.WriteResponseStats{}, nil
+func (prw *prometheusRemoteWriteReceiver) translateV2(_ context.Context, req *writev2.Request) (pmetric.Metrics, promremote.WriteResponseStats, error) {
+	var (
+		badRequestErrors []error


can we just use a single error, and use errors.Join to append new errors? e.g.

var err error // then, when we want to append an error: err = errors.Join(err, fmt.Errorf("missing metric name in labels"))

dashpole · 2024-11-11T21:15:14Z

receiver/prometheusremotewritereceiver/receiver.go

+	var (
+		badRequestErrors []error
+		otelMetrics      = pmetric.NewMetrics()
+		b                = labels.NewScratchBuilder(0)


nit: can we use a more descriptive name? Perhaps labelsBuilder?

dashpole · 2024-11-11T21:19:51Z

receiver/prometheusremotewritereceiver/receiver.go

@@ -39,8 +42,9 @@ type prometheusRemoteWriteReceiver struct {
 	settings     receiver.Settings
 	nextConsumer consumer.Metrics

-	config *Config
-	server *http.Server
+	jobInstanceCache map[string]pmetric.ResourceMetrics


consider using an int64 as the key for this cache, and constructing an identifier using xxhash, similar to what prometheus translation does: https://github.com/prometheus/prometheus/blob/0f0deb77a2f15383914ad60a3309281adb7e8b27/storage/remote/otlptranslator/prometheusremotewrite/helper.go#L94

You can just append the job, a separator, and then the instance.

dashpole · 2024-11-11T21:20:51Z

receiver/prometheusremotewritereceiver/receiver.go

@@ -39,8 +42,9 @@ type prometheusRemoteWriteReceiver struct {
 	settings     receiver.Settings
 	nextConsumer consumer.Metrics

-	config *Config
-	server *http.Server
+	jobInstanceCache map[string]pmetric.ResourceMetrics


Do we have any mechanism to expire items in this cache? We don't want this to leak memory.

Actually, i'm not sure we should be storing the job + instance cache on the receiver itself. We probably want this cache around for a single PRW request, right? Otherwise, we will keep appending new metrics to the same resourceMetrics forever.

dashpole · 2024-11-11T21:23:24Z

receiver/prometheusremotewritereceiver/receiver.go

+// Following the specification at https://opentelemetry.io/docs/specs/otel/compatibility/prometheus_and_openmetrics/
+func parseJobAndInstance(dest pcommon.Map, instance, job string) {
+	if job != "" {
+		dest.PutStr("service.namespace", job)


Job should be service.name + "/" + service.namespace. Instance is only service.instance.id. You need to move strings.Split(instance, "/") to the job case.

ArthurSens requested review from dashpole and a team as code owners October 7, 2024 18:34

github-actions bot assigned mwear Oct 7, 2024

github-actions bot added the receiver/prometheusremotewrite label Oct 7, 2024

ArthurSens force-pushed the prwreceiver-parselabels branch 4 times, most recently from 5f5d327 to b36ce75 Compare October 8, 2024 13:05

ArthurSens force-pushed the prwreceiver-parselabels branch from b36ce75 to 17df302 Compare October 18, 2024 14:38

ArthurSens force-pushed the prwreceiver-parselabels branch from 17df302 to 3854a92 Compare October 30, 2024 13:55

ArthurSens marked this pull request as draft October 30, 2024 13:55

ArthurSens force-pushed the prwreceiver-parselabels branch from 3854a92 to 1d30b01 Compare October 30, 2024 15:30

ArthurSens changed the title ~~[receiver/prometheusremotewrite] Parse labels into resource attributes~~ [receiver/prometheusremotewrite] Parse labels Oct 30, 2024

ArthurSens force-pushed the prwreceiver-parselabels branch from 1d30b01 to b59d07e Compare October 30, 2024 15:37

ArthurSens marked this pull request as ready for review October 30, 2024 15:37

github-actions bot assigned andrzej-stencel Oct 30, 2024

ArthurSens force-pushed the prwreceiver-parselabels branch from b59d07e to 1c9ff80 Compare October 30, 2024 15:39

dashpole reviewed Oct 31, 2024

View reviewed changes

receiver/prometheusremotewritereceiver/receiver.go Outdated Show resolved Hide resolved

dashpole reviewed Oct 31, 2024

View reviewed changes

ArthurSens requested review from djaglowski, jmacd, dmitryax, TylerHelmuth, yurishkuro and codeboten as code owners November 4, 2024 21:08

ArthurSens requested review from atoulme, MovieStoreGuy, andrzej-stencel, crobert-1, jpkrohling, mx-psi, bogdandrutu, evan-bradley, tigrannajaryan, jsuereth, songy23 and mwear as code owners November 4, 2024 21:08

github-actions bot added cmd/opampsupervisor connector/datadog connector/routing labels Nov 4, 2024

github-actions bot requested review from ankitpatel96 and dineshg13 November 4, 2024 21:09

ArthurSens force-pushed the prwreceiver-parselabels branch from 1ecb481 to 97d203a Compare November 4, 2024 21:16

ArthurSens force-pushed the prwreceiver-parselabels branch 3 times, most recently from f902047 to b1e96fa Compare November 7, 2024 00:38

[receiver/prometheusremotewrite] Parse labels into resource and metri…

f862fe3

…c attributes Signed-off-by: Arthur Silva Sens <[email protected]>

ArthurSens force-pushed the prwreceiver-parselabels branch from b1e96fa to f862fe3 Compare November 7, 2024 12:49

dashpole reviewed Nov 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[receiver/prometheusremotewrite] Parse labels #35656

[receiver/prometheusremotewrite] Parse labels #35656

ArthurSens commented Oct 7, 2024 •

edited

Loading

dashpole commented Oct 9, 2024

ArthurSens commented Oct 13, 2024

dashpole commented Oct 14, 2024

ArthurSens commented Oct 17, 2024

ArthurSens commented Oct 30, 2024 •

edited

Loading

dashpole Oct 31, 2024

ArthurSens commented Nov 4, 2024

dashpole Nov 11, 2024

dashpole Nov 11, 2024

dashpole Nov 11, 2024

dashpole Nov 11, 2024

dashpole Nov 11, 2024

dashpole Nov 11, 2024

[receiver/prometheusremotewrite] Parse labels #35656

Are you sure you want to change the base?

[receiver/prometheusremotewrite] Parse labels #35656

Conversation

ArthurSens commented Oct 7, 2024 • edited Loading

Description

dashpole commented Oct 9, 2024

ArthurSens commented Oct 13, 2024

dashpole commented Oct 14, 2024

ArthurSens commented Oct 17, 2024

ArthurSens commented Oct 30, 2024 • edited Loading

dashpole Oct 31, 2024

Choose a reason for hiding this comment

ArthurSens commented Nov 4, 2024

dashpole Nov 11, 2024

Choose a reason for hiding this comment

dashpole Nov 11, 2024

Choose a reason for hiding this comment

dashpole Nov 11, 2024

Choose a reason for hiding this comment

dashpole Nov 11, 2024

Choose a reason for hiding this comment

dashpole Nov 11, 2024

Choose a reason for hiding this comment

dashpole Nov 11, 2024

Choose a reason for hiding this comment

ArthurSens commented Oct 7, 2024 •

edited

Loading

ArthurSens commented Oct 30, 2024 •

edited

Loading