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

prometheus receiver handling of lone _created metrics #30309

Closed
seankhliao opened this issue Jan 5, 2024 · 6 comments
Closed

prometheus receiver handling of lone _created metrics #30309

seankhliao opened this issue Jan 5, 2024 · 6 comments
Assignees
Labels
bug Something isn't working receiver/prometheus Prometheus receiver

Comments

@seankhliao
Copy link
Contributor

Component(s)

receiver/prometheus

What happened?

Description

Metrics from a prometheus (but not openmetrics) client library ending with _created don't have their values set.

Steps to Reproduce

Run go program:

package main

import (
	"net/http"

	"github.com/prometheus/client_golang/prometheus"
	"github.com/prometheus/client_golang/prometheus/promauto"
	"github.com/prometheus/client_golang/prometheus/promhttp"
)

func main() {
	ctr := promauto.NewCounter(prometheus.CounterOpts{
		Name: "my_thing_created",
	})
	ctr.Add(300)
	http.ListenAndServe(":8080", promhttp.Handler())
}

Run collector (see below for config)

Expected Result

A metric with name my_thing_created and value 300.

Actual Result

a metric with name my_thing_created and value 0, with a wrong start timestamp.

2024-01-05T20:22:18.263Z	info	ResourceMetrics #0
Resource SchemaURL: 
Resource attributes:
     -> service.name: Str(app)
     -> service.instance.id: Str(127.0.0.1:8080)
     -> net.host.port: Str(8080)
     -> http.scheme: Str(http)
ScopeMetrics #0
ScopeMetrics SchemaURL: 
InstrumentationScope otelcol/prometheusreceiver 0.91.0
Metric #0
Descriptor:
     -> Name: my_thing_created
     -> Description: 
     -> Unit: 
     -> DataType: Sum
     -> IsMonotonic: true
     -> AggregationTemporality: Cumulative
NumberDataPoints #0
StartTimestamp: 1970-01-01 00:05:00 +0000 UTC
Timestamp: 2024-01-05 20:22:18.259 +0000 UTC
Value: 0.000000

Collector version

otelcol-contrib version 0.91.0

Environment information

OpenTelemetry Collector configuration

receivers:
  prometheus:
    config:
      scrape_configs:
        - job_name: 'app'
          scrape_interval: 5s
          static_configs:
            - targets: ['127.0.0.1:8080']
  
processors:
  filter:
    metrics:
      metric:
        - name != "my_thing_created"

exporters:
  debug:
    verbosity: detailed

service:
  pipelines:
    metrics:
      receivers:
        - prometheus
      processors:
        - filter
      exporters:
        - debug

Log output

-

Additional context

The metric looks like this in the scrape output:

# HELP my_thing_created 
# TYPE my_thing_created counter
my_thing_created 300

OpenMetrics uses _created as one of the special suffixes for defining the start timestamp of a given time series: https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md#suffixes

given the TYPE hint, the metric shouldn't be treated as a timestamp, since it's an exact match, not with an extra _created suffix appended.

It looks like the collector is treating the metric as a timestamp (300s == 5 min past unix epoch), but also for the wrong metric name, since it it were a timestamp, it should be for a metric named my_metric instead of my_metric_created

@seankhliao seankhliao added bug Something isn't working needs triage New item requiring triage labels Jan 5, 2024
@github-actions github-actions bot added the receiver/prometheus Prometheus receiver label Jan 5, 2024
Copy link
Contributor

github-actions bot commented Jan 5, 2024

Pinging code owners:

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

@dashpole dashpole self-assigned this Jan 8, 2024
@dashpole dashpole removed the needs triage New item requiring triage label Jan 8, 2024
@dashpole
Copy link
Contributor

dashpole commented Jan 8, 2024

Thanks for the report. I'll look into it when I have time.

@jmichalek132
Copy link
Contributor

jmichalek132 commented Jan 13, 2024

So I took a look at this, and found the cause here:

if strings.HasSuffix(metricName, metricSuffixCreated) {

if metrics has the _created suffix, it's value isn't set, instead created gets set.
In theory we could do something like

if strings.HasSuffix(metricName, metricSuffixCreated) && (mf.metadata.Type == "timestamp" || mf.metadata.Type == "") {
    mg.created = v
} else {
    mg.value = v
}

but I am not sure if that's the best approach.
I'll raise this as a topic in the OpenTelemetry Prometheus Workgroup.

@dashpole
Copy link
Contributor

I think we would need to ensure that the suffix is in addition to the normal metric name (that we looked up in the metadata cache).

@dashpole
Copy link
Contributor

Openmetrics spec demonstrating created timestamp: https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md#overall-structure

Discussed this at the SIG meeting. If the metricfamily name + _created == the series name, then we should use it as a created timestamp.

@dashpole dashpole assigned jmichalek132 and unassigned dashpole Jan 31, 2024
codeboten pushed a commit that referenced this issue Feb 13, 2024
Description:
As discussed in the OTEL Prometheus working group, to fix this case we
check If the metricfamily (which we get from the metadata) name +
_created == the series name match. Then we treat it as created
timestamp.
Link to tracking Issue:
#30309

Testing: Reproduced based on the details provided in the issue, added a
test case covering the case.

Documentation:

---------

Co-authored-by: David Ashpole <[email protected]>
XinRanZhAWS pushed a commit to XinRanZhAWS/opentelemetry-collector-contrib that referenced this issue Mar 13, 2024
)

Description:
As discussed in the OTEL Prometheus working group, to fix this case we
check If the metricfamily (which we get from the metadata) name +
_created == the series name match. Then we treat it as created
timestamp.
Link to tracking Issue:
open-telemetry#30309

Testing: Reproduced based on the details provided in the issue, added a
test case covering the case.

Documentation:

---------

Co-authored-by: David Ashpole <[email protected]>
@jmichalek132
Copy link
Contributor

This was fixed in #30808, could you please close this one @dashpole ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working receiver/prometheus Prometheus receiver
Projects
None yet
Development

No branches or pull requests

3 participants