Skip to content
This repository has been archived by the owner on Nov 21, 2024. It is now read-only.

Remote write compliance: TestRemoteWrite/otelcollector/RepeatedLabels #44

Closed
rakyll opened this issue Apr 20, 2021 · 11 comments · Fixed by open-telemetry/opentelemetry-collector#3408
Assignees
Labels
phase1 phase1 tasks prom-exporter Prometheus exporter tasks

Comments

@rakyll
Copy link
Contributor

rakyll commented Apr 20, 2021

The OpenTelemetry collector is not passing https://github.com/prometheus/compliance/tree/main/remote_write TestRemoteWrite/otelcollector/RepeatedLabels.

=== CONT  TestRemoteWrite/otelcollector/RepeatedLabels
    labels.go:58:
        	Error Trace:	labels.go:58
        	            				main_test.go:101
        	            				main_test.go:65
        	Error:      	Should be true
        	Test:       	TestRemoteWrite/otelcollector/RepeatedLabels
        	Messages:   	found zero samples for up{job="test"} = 0
@rakyll
Copy link
Contributor Author

rakyll commented Apr 21, 2021

Related to #41.

@Aneurysm9
Copy link
Member

This continues to fail even with the up metric present:

=== CONT  TestRemoteWrite/otelcollector/RepeatedLabels
    labels.go:58:
                Error Trace:    labels.go:58
                                                        main_test.go:105
                                                        main_test.go:69
                Error:          Should be true
                Test:           TestRemoteWrite/otelcollector/RepeatedLabels
                Messages:       found samples for test{a="1"}, none expected

@Aneurysm9 Aneurysm9 reopened this Jun 8, 2021
@odeke-em
Copy link
Member

odeke-em commented Jun 8, 2021

Please assign this issue to me

@alolita
Copy link
Member

alolita commented Jun 8, 2021

All yours @odeke-em

@odeke-em
Copy link
Member

odeke-em commented Jun 9, 2021

I think the Prometheus Remote Write compliance tests are wrong. I have written a sample test and used Prometheus with it and Prometheus is able to successfully get the datapoints.

main.go

package main

import (
	"context"
	"fmt"
	"net/http"
	"os"
	"os/signal"
	"sync/atomic"
	"time"
)

func main() {
	ctx, cancel := context.WithCancel(context.Background())
	i := uint64(1)

	go func() {
		for {
			select {
			case <-ctx.Done():
				return
			case <-time.After(27 * time.Second):
				atomic.AddUint64(&i, 1)
			}
		}
	}()

	handler := func(rw http.ResponseWriter, req *http.Request) {
		println("Scraped", time.Now().Unix())
		fmt.Fprintf(rw, `
# HELP test A gauge
# TYPE test gauge
test{a="1",a="1"} %.1f`, float64(atomic.LoadUint64(&i)))
	}

	sigCh := make(chan os.Signal, 1)
	signal.Notify(sigCh, os.Interrupt)

	defer cancel()

	go func() {
		if err := http.ListenAndServe(":8855", http.HandlerFunc(handler)); err != nil {
			panic(err)
		}
	}()
	<-sigCh
}

Run it by go run main.go

prom.yaml

global:
  scrape_interval: 10s

  external_labels:
    monitor: 'otlc'

remote_write:

scrape_configs:
  - job_name: 'otlc'

    scrape_interval: 10s

    honor_labels: true

    static_configs:
        - targets: ['localhost:8855']

Run it by prometheus --config.file=prom.yaml --log.level=debug

Viewed on Prometheus

Screen Shot 2021-06-09 at 1 45 02 PM

Screen Shot 2021-06-09 at 1 45 16 PM

Synopsis

Prometheus itself doesn't reject these repeated labels, so why are the compliance tests rejecting it? Kindly cc-ing @tomwilkie @brian-brazil @rakyll @Aneurysm9 @alolita.

@brian-brazil
Copy link

Prometheus itself doesn't reject these repeated labels

If that's the case, then that's a bug in Prometheus. Are you sure you're using the latest version? We had to fix this at one point.

@roidelapluie
Copy link

Your Prometheus screenshot seems from a release before Prometheus 2.14, because it is the "old" UI without a link to the new UI. That link was added in 2.14. I guess you are using a Prometheus release that was released before 2019-11-11.

Duplicate labels are rejected since 2.16.0, released on 2020-02-13.

@odeke-em
Copy link
Member

odeke-em commented Jun 9, 2021 via email

@odeke-em
Copy link
Member

odeke-em commented Jun 9, 2021

Yup, with the latest, I now see the issue

level=debug ts=2021-06-09T12:45:39.268Z caller=scrape.go:1459 component="scrape manager" scrape_pool=otlc target=http://localhost:8855/metrics msg="Unexpected error" series="test{a=\"1\",a=\"1\"}" err="label name \"a\" is not unique: invalid sample"
level=debug ts=2021-06-09T12:45:39.268Z caller=scrape.go:1247 component="scrape manager" scrape_pool=otlc target=http://localhost:8855/metrics msg="Append failed" err="label name \"a\" is not unique: invalid sample"

Thank you Brian and Julien!

@odeke-em
Copy link
Member

odeke-em commented Jun 9, 2021

This issue was a red herring, the problem is actually in the Prometheus receiver and I've filed open-telemetry/opentelemetry-collector#3407 and am mailing a PR soon after the Prometheus Working Group meeting that's currently ongoing.

odeke-em added a commit to orijtech/opentelemetry-collector that referenced this issue Jun 9, 2021
As of Prometheus 2.16.0 (released on 2020-02-13), datapoints with
duplicate label keys MUST be rejected and the Prometheus RemoteWrite
exporter tests were failing, but that was a red herring as the
real issue was really in the receiver.

Fixes open-telemetry/prometheus-interoperability-spec#44
Fixes open-telemetry#3407
odeke-em added a commit to orijtech/opentelemetry-collector that referenced this issue Jun 9, 2021
As of Prometheus 2.16.0 (released on 2020-02-13), datapoints with
duplicate label keys MUST be rejected and the Prometheus RemoteWrite
exporter tests were failing, but that was a red herring as the
real issue was really in the receiver.

Fixes open-telemetry/prometheus-interoperability-spec#44
Fixes open-telemetry#3407
bogdandrutu pushed a commit to open-telemetry/opentelemetry-collector that referenced this issue Jun 10, 2021
As of Prometheus 2.16.0 (released on 2020-02-13), datapoints with
duplicate label keys MUST be rejected and the Prometheus RemoteWrite
exporter tests were failing, but that was a red herring as the
real issue was really in the receiver.

Fixes open-telemetry/prometheus-interoperability-spec#44
Fixes #3407
@alolita alolita added this to the Phase 1 Implementation milestone Jun 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
phase1 phase1 tasks prom-exporter Prometheus exporter tasks
Projects
None yet
6 participants