[exporter][prometheusremotewrite] Additional metrics for the exporter #37912

gotjosh · 2025-02-13T19:49:18Z

Description

Often, we have to do anylsis of the current state of the exporter which includes the current level of concurrency and throughput.

For the concurrency, we need to rely on revising each individual configuration which is time consuming, here I'm proposing to have it as a metric that we can keep a record of and see it through observability.

For the throughput, while tecnically otelcol_processor_batch_batch_send_size_count exists, this is not divisible by type (e.g. metrics vs traces) and is not specific to exporter. Whilst I'm also interested in splitting batch_send_size_count by type (because that also includes OTLP batches and this metric is only for RW), I think this is part of a separate conversation and it doesn't hurt to have the single counter that's specific for remote write.

There's one more thing here (and please advise if you'd rather see a different PR for it) - I'm also adding the url as attribute to ALL the metrics as I think this is key to identify where the exporter is pointing to.

This is tecnically my first contribution to the repo so I'd appreciate any guidiance on what else can I need to do. Telemetry tests appear to be auto generated so I didn't add any to this PR but please let me know otherwise.

Link to tracking issue

This technically a prequite for the work that I'm about to do as part of open-telemetry/opentelemetry-collector#12255 but is not directly related and can go individually.

Signed-off-by: gotjosh josue.abreu@gmail.com

linux-foundation-easycla · 2025-02-13T19:49:22Z

The committers listed above are authorized under a signed CLA.

✅ login: gotjosh / name: gotjosh (f535f8e, b807d54, d459f90, dd3c466, e4548e9, fba30db, 62bcf74, 10ffb75, 375aed2, 36acb44)
✅ login: andrzej-stencel / name: Andrzej Stencel (9cae33c)
✅ login: MovieStoreGuy / name: Sean Marciniak (69cd5ed)

ArthurSens

I had a super quick look, I'll do proper testing and review tomorrow but I had one doubt already

ArthurSens · 2025-02-13T21:55:10Z

exporter/prometheusremotewriteexporter/exporter.go

@@ -101,6 +111,7 @@ func newPRWTelemetry(set exporter.Settings) (prwTelemetry, error) {
 		telemetryBuilder: telemetryBuilder,
 		otelAttrs: []attribute.KeyValue{
 			attribute.String("exporter", set.ID.String()),
+			attribute.String("endpoint", endpointURL.String()),


If we add this attribute to otelAttrs, will this be added to all metrics of the exporter? Or just the new ones you're adding in the PR?

Depends on what you mean with "all metrics of the exporter".

It'll certainly be added to all metrics defined specifically for the exporter but not the generic ones, in this case we'll add them to:

otelcol_exporter_prometheusremotewrite_consumers

otelcol_exporter_prometheusremotewrite_sent_batch_count

otelcol_exporter_prometheusremotewrite_translated_time_series

otelcol_exporter_prometheusremotewrite_failed_translations

but not to other metrics such asotelcol_exporter_queue_capacity or otelcol_exporter_send_failed_metric_points

Often, we have to do anylsis of the current state of the exporter which includes the current level of concurrency and throughput. For the concurrency, we need to rely on revising each individual configuration which is time consuming, here I'm proposing to have it as a metric that we can keep a record of and see it through observability. For the throughput, while tecnically `otelcol_processor_batch_batch_send_size_count` exists, this is not divisible by type (e.g. metrics vs traces) and is not specific to exporter. Whilst I'm also interested in splitting `batch_send_size_count` by type (because that also includes OTLP batches and this metric is only for RW), I think this is part of a separate conversation and it doesn't hurt to have the single counter that's specific for remote write. There's one more thing here (and please advise if you'd rather see a different PR for it) - I'm also adding the `url` as attribute to _ALL_ the metrics as I think this is key to identify where the exporter is pointing to. This is tecnically my first contribution to the repo so I'd appreciate any guidiance on what else can I need to do. Telemetry tests appear to be auto generated so I didn't add any to this PR but please let me know otherwise. Signed-off-by: gotjosh <josue.abreu@gmail.com>

the exporter automatically adds `total` at the end so I think this is good enough.

gotjosh · 2025-02-14T14:52:28Z

I think this is now ready for reviewed, I have manually tested my changes and they look from my side.

Does this need a changelog entry? I couldn't tell if we do changelog entries for new metrics.

gotjosh · 2025-02-14T16:26:09Z

exporter/prometheusremotewriteexporter/exporter.go

-	prwTelemetry, err := newPRWTelemetry(set)
+	telemetry, err := newPRWTelemetry(set, endpointURL)


renamed this variable because it was shadowing a package name.

ArthurSens

I think we should add the remoteWriteURL as an attribute in a different PR, it's very easy to split and we can easily rollback PRs in case something unrelated goes wrong.

Could someone with permissions trigger the CI?

exporter/prometheusremotewriteexporter/exporter.go

Signed-off-by: gotjosh <josue.abreu@gmail.com>

gotjosh · 2025-02-14T18:03:41Z

I think we should add the remoteWriteURL as an attribute in a different PR, it's very easy to split and we can easily rollback PRs in case something unrelated goes wrong.

Done in f535f8e

We don't need it anymore Signed-off-by: gotjosh <josue.abreu@gmail.com>

Signed-off-by: gotjosh <josue.abreu@gmail.com>

gotjosh · 2025-02-15T14:41:00Z

Thanks @ArthurSens, I would appreciate if anyone could please kick in the CI again - I believe I have addressed everything now that I have figured out how to run the test / linters locally.

Signed-off-by: gotjosh <josue.abreu@gmail.com>

ArthurSens

LGTM

exporter/prometheusremotewriteexporter/metadata.yaml

Signed-off-by: gotjosh <josue.abreu@gmail.com>

dashpole · 2025-02-19T17:13:28Z

exporter/prometheusremotewriteexporter/documentation.md

@@ -14,6 +22,14 @@ Number of translation operations that failed to translate metrics from Otel to P
 | ---- | ----------- | ---------- | --------- |
 | 1 | Sum | Int | true |

+### otelcol_exporter_prometheusremotewrite_sent_batch


Based on https://github.com/open-telemetry/semantic-conventions/blob/e70853397b73ca9cb85b1df45182b6a3e299c85e/docs/general/naming.md#naming-rules-for-counters-and-updowncounters, it seems like this should be batches, rather than batch

Should I also re-add count which I had previously removed thinking it was redudant?

Use count Instead of Pluralization for UpDownCounters

This is a Counter, not an updowncounter, so the updowncounter guidance does not apply here

Metric names SHOULD NOT be pluralized, unless the value being recorded represents discrete instances of a countable quantity. Generally, the name SHOULD be pluralized only if the unit of the metric in question is a non-unit (like {fault} or {operation}).

This is a count of discrete instances of a countable quantity, so it should be plural.

Done in dd3c466

Appreciate the feedback - can you please kick off the CI again?

Signed-off-by: gotjosh <josue.abreu@gmail.com>

.chloggen/additional-metrics-for-remote-write-exporter.yaml

dehaansa · 2025-02-28T03:26:08Z

exporter/prometheusremotewriteexporter/metadata.yaml

+      enabled: true
+      description: Number of configured workers to use to fan out the outgoing requests
+      unit: "{consumer}"
+      sum:


Why is this a sum, and not a gauge? In my understanding this should be a value representing the current configuration, not a counter/sum of any form.

I thought the same 😄 , but @dashpole and I had a conversation about it: #37912 (comment)

Let me know what you think.

I think the part that was mostly throwing me off was using "Add" to set the value, but I see that it's only set on exporter initialization, so functionally it should be fine. 👍

jmichalek132

LGTM.

Signed-off-by: gotjosh <josue.abreu@gmail.com>

github-actions bot added the exporter/prometheusremotewrite label Feb 13, 2025

github-actions bot requested review from Aneurysm9, ArthurSens, dashpole and rapphil February 13, 2025 19:49

ArthurSens reviewed Feb 13, 2025

View reviewed changes

gotjosh force-pushed the additional-metrics-for-remote-write-exporter branch from 942ecaf to d459f90 Compare February 14, 2025 09:22

remove the count suffix

10ffb75

the exporter automatically adds `total` at the end so I think this is good enough.

gotjosh marked this pull request as ready for review February 14, 2025 14:51

gotjosh requested a review from a team as a code owner February 14, 2025 14:51

github-actions bot assigned codeboten Feb 14, 2025

gotjosh commented Feb 14, 2025

View reviewed changes

ArthurSens reviewed Feb 14, 2025

View reviewed changes

exporter/prometheusremotewriteexporter/exporter.go Outdated Show resolved Hide resolved

Remove the attribute and fix metric name

f535f8e

Signed-off-by: gotjosh <josue.abreu@gmail.com>

gotjosh added 3 commits February 14, 2025 19:09

Remove the endpointURL from the arguments to telemetry

b807d54

We don't need it anymore Signed-off-by: gotjosh <josue.abreu@gmail.com>

add the telemetry to the test

e4548e9

Signed-off-by: gotjosh <josue.abreu@gmail.com>

Merge branch 'main' into additional-metrics-for-remote-write-exporter

375aed2

Add a changelog entry

fba30db

Signed-off-by: gotjosh <josue.abreu@gmail.com>

ArthurSens approved these changes Feb 18, 2025

View reviewed changes

dashpole reviewed Feb 19, 2025

View reviewed changes

exporter/prometheusremotewriteexporter/metadata.yaml Outdated Show resolved Hide resolved

dashpole reviewed Feb 19, 2025

View reviewed changes

exporter/prometheusremotewriteexporter/metadata.yaml Outdated Show resolved Hide resolved

dashpole reviewed Feb 19, 2025

View reviewed changes

exporter/prometheusremotewriteexporter/metadata.yaml Outdated Show resolved Hide resolved

address PR feedback from david

62bcf74

Signed-off-by: gotjosh <josue.abreu@gmail.com>

dashpole reviewed Feb 19, 2025

View reviewed changes

counters should end with a pluralization

dd3c466

Signed-off-by: gotjosh <josue.abreu@gmail.com>

Merge branch 'main' into additional-metrics-for-remote-write-exporter

69cd5ed

dehaansa reviewed Feb 28, 2025

View reviewed changes

jmichalek132 approved these changes Feb 28, 2025

View reviewed changes

address review feedback

36acb44

Signed-off-by: gotjosh <josue.abreu@gmail.com>

andrzej-stencel approved these changes Mar 3, 2025

View reviewed changes

Merge branch 'main' into additional-metrics-for-remote-write-exporter

9cae33c

andrzej-stencel added the ready to merge label Mar 3, 2025

andrzej-stencel merged commit f2747bd into open-telemetry:main Mar 3, 2025
168 checks passed

github-actions bot added this to the next release milestone Mar 3, 2025

gotjosh deleted the additional-metrics-for-remote-write-exporter branch March 3, 2025 15:01

gotjosh mentioned this pull request Mar 5, 2025

[exporter/prometheusremotewrite] Add endpoint label to the remote write metrics #38397

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[exporter][prometheusremotewrite] Additional metrics for the exporter #37912

[exporter][prometheusremotewrite] Additional metrics for the exporter #37912

gotjosh commented Feb 13, 2025 •

edited

Loading

linux-foundation-easycla bot commented Feb 13, 2025 •

edited

Loading

ArthurSens left a comment

ArthurSens Feb 13, 2025

gotjosh Feb 14, 2025

gotjosh Feb 14, 2025

gotjosh commented Feb 14, 2025

gotjosh Feb 14, 2025

ArthurSens left a comment •

edited

Loading

gotjosh commented Feb 14, 2025

gotjosh commented Feb 15, 2025 •

edited

Loading

ArthurSens left a comment

dashpole Feb 19, 2025

gotjosh Feb 19, 2025 •

edited

Loading

dashpole Feb 19, 2025

gotjosh Feb 19, 2025

dehaansa Feb 28, 2025

gotjosh Mar 2, 2025

dehaansa Mar 3, 2025

jmichalek132 left a comment

		prwTelemetry, err := newPRWTelemetry(set)
		telemetry, err := newPRWTelemetry(set, endpointURL)

[exporter][prometheusremotewrite] Additional metrics for the exporter #37912

[exporter][prometheusremotewrite] Additional metrics for the exporter #37912

Conversation

gotjosh commented Feb 13, 2025 • edited Loading

Description

Link to tracking issue

linux-foundation-easycla bot commented Feb 13, 2025 • edited Loading

ArthurSens left a 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

gotjosh commented Feb 14, 2025

Choose a reason for hiding this comment

ArthurSens left a comment • edited Loading

Choose a reason for hiding this comment

gotjosh commented Feb 14, 2025

gotjosh commented Feb 15, 2025 • edited Loading

ArthurSens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gotjosh Feb 19, 2025 • edited Loading

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

jmichalek132 left a comment

Choose a reason for hiding this comment

gotjosh commented Feb 13, 2025 •

edited

Loading

linux-foundation-easycla bot commented Feb 13, 2025 •

edited

Loading

ArthurSens left a comment •

edited

Loading

gotjosh commented Feb 15, 2025 •

edited

Loading

gotjosh Feb 19, 2025 •

edited

Loading