[exporter] prometheusremotewrite appends _None to Datapoints with "Unit: None"

[disfluxly]

Component(s)

exporter/prometheusremotewrite

What happened?

Description

It seems that datapoints where the Unit has not been assigned (Unit: None) end up having _None appended to the end of the metric name.

Steps to Reproduce

1. Send metrics to a receiver without specifying a Unit Type (the ecscontainermetrics receiver doesn't set Unit for some CPU Metrics, as shown in the screenshot below)
2. Use the prometheusremotewrite exporter, which will append _None to the metrics.

Expected Result

_None not being appended to Metrics

Actual Result

_None is being appended to Metrics

Collector version

0.77.0

Environment information

Environment

Linux - Alpine

OpenTelemetry Collector configuration

receivers:
  awsecscontainermetrics:
    collection_interval: 20s
exporters:
  logging:
    verbosity: detailed
    sampling_initial: 5
    sampling_thereafter: 200
  prometheusremotewrite:
    endpoint: "http:https://prometheus/api/v1/push"
    tls:
      insecure: true
      cert_file: file.cert
      key_file: file.key
    resource_to_telemetry_conversion:
      enabled: true
    target_info:
      enabled: false
    export_created_metric:
      enabled: false
processors:
  memory_limiter:
    check_interval: 1s
    limit_percentage: 80
    spike_limit_percentage: 20
  batch:
    send_batch_size: 8192
    timeout: 200ms
    send_batch_max_size: 0
service:
  pipelines:
    metrics:
      receivers: [awsecscontainermetrics]
      processors: [memory_limiter,batch]
      exporters: [prometheusremotewrite,logging]

Log output

Descriptor:
-> Name: container.cpu.utilized
-> Description:
-> Unit: None
-> DataType: Gauge
...
Descriptor:
-> Name: container.cpu.reserved
-> Description:
-> Unit: None
-> DataType: Gauge

### Additional context

Metrics showing in Mimir:

![image](https://github.com/open-telemetry/opentelemetry-collector-contrib/assets/31159882/95d657e3-5739-4570-ae55-fa1d0ee530ed)

[github-actions]

Pinging code owners:

• exporter/prometheusremotewrite: @Aneurysm9 @rapphil

See Adding Labels via Comments if you do not have permissions to add labels yourself.

[frzifus]

Looks like the formatting is a bit broken. But here is your screenshot:

[mx-psi]

Is 'None' a valid unit according to the UCUM specification? I am not very knowledgeable about it, but have always seen 1 to represent lack of units.

If None is invalid, the problem is in the receiver instead of the exporter.

[disfluxly]

@mx-psi - Those metrics I posted above are actually coming from the ecscontainermetrics receiver, not the dockerstats receiver.

Either way, I don't think any exporter should be mutating metric names, regardless of the unit inference. Doing so brings about an aspect of chaos that brings into question the consistency/validity of the metrics, and especially impacts cross-version compatibility.

[mx-psi]

@disfluxly

Those metrics I posted above are actually coming from the ecscontainermetrics receiver, not the dockerstats receiver

Sorry, I posted the wrong link, I meant to link https://ucum.org, it should be fixed now :) In any case, I meant that whatever receiver is producing these is probably at fault.

I don't think any exporter should be mutating metric names, regardless of the unit inference. Doing so brings about an aspect of chaos that brings into question the consistency/validity of the metrics, and especially impacts cross-version compatibility.

As I understand it, this is something that should be done according to the opentelemetry specification:

The resulting unit SHOULD be added to the metric as OpenMetrics UNIT metadata and as a suffix to the metric name unless the metric name already contains the unit, or the unit MUST be omitted. The unit suffix comes before any type-specific suffixes.

If you think the spec should be changed or you want to understand why it works that way, I would suggest you open an issue over at https://github.com/open-telemetry/opentelemetry-specification; we won't change the behavior of the PRW exporter unless the specification itself changes.

[disfluxly]

@mx-psi - Dang, so that's effectively going to break every single metric that has previously passed through the exporters.

I think the PRW exporter should support this use case:

or the unit MUST be omitted

as a configuration value. What are your thoughts on that? That would adhere to PRW exporter to both parts of the specification.

[mx-psi]

I don't have a strong opinion about this, I will defer to @Aneurysm9 and @rapphil

[disfluxly]

I suppose I could use the transform processor to effectively strip out the Unit on all of them, assuming that the exporter doesn't then append a _None.

[disfluxly]

Looks like this is the relevant discussion: #21743

[disfluxly]

Going to close this issue as this should ultimately be resolved by the decision made in the issue linked in the comment above ^