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

Use a map to describe discovered labels, as they are not validated by the server #529

Merged
merged 1 commit into from Jan 23, 2019
Merged

Use a map to describe discovered labels, as they are not validated by the server #529

merged 1 commit into from Jan 23, 2019

Conversation

okushchenko
Copy link
Contributor

@okushchenko okushchenko commented Jan 21, 2019

The /targets endpoint generates a JSON decoding error when you try to send a request to it while having the Node Exporter configured in Prometheus. It looks like discovered labels are being presented as is, without any validation. In particular I received this error: "__param_collect[]" is not a valid label name.

Here is the part of Prometheus server code that uses a map[string]string to describe discovered labels internally.
https://github.com/prometheus/prometheus/blob/a1f34bec2e6584a2fee9aec901f3157e3e12cbaa/web/api/v1/api.go#L505-L517

… the server

Signed-off-by: Oleksandr Kushchenko <okushchenko@palantir.com>
@beorn7
Copy link
Member

beorn7 commented Jan 21, 2019

@krasi-georgiev this one is for you.

@krasi-georgiev
Copy link
Contributor

@okushchenko thanks the change looks valid.
can you provide some code how are you using it as I am a bit confused that you mention the /rules endpoint but the PR adds changes for the ActiveTargets

@okushchenko
Copy link
Contributor Author

@krasi-georgiev sorry, that was just a typo. I'm using both of these endpoints, but only one of them (/targets) fails. The code example (parameters initialization was omitted for simplicity):

import (
	promapiv1 "github.com/prometheus/client_golang/api/prometheus/v1"
)

promAPI := promapiv1.NewAPI(client)
err = promAPI.Targets(ctx)
if err != nil {
	return err
}

@krasi-georgiev
Copy link
Contributor

krasi-georgiev commented Jan 23, 2019

at my end I don't get any errors and get the targets as expected when I run:

func TestTargets(t *testing.T) {
	client, err := api.NewClient(api.Config{
		Address: "http://127.0.0.1:9090",
	})
	if err != nil {
		t.Fatal("error creating HTTP client", err)
	}
	promAPI := v1.NewAPI(client)
	res, err := promAPI.Targets(context.Background())
	if err != nil {
		t.Fatal("getting targets", err)
	}
	fmt.Println(res)
}

Prometheus is running at http://127.0.0.1:9090

@okushchenko
Copy link
Contributor Author

okushchenko commented Jan 23, 2019

@krasi-georgiev in order to reproduce this issue you can use the node_exporter registered as a target in Prometheus and use a list parameters to filter collectors to recreate this issue: https://github.com/prometheus/node_exporter#filtering-enabled-collectors
These scrape parameters are then being used by Prometheus as URL encoded parameters to scrape Node Exporter. These params are also apparently being present in DiscoveredLabels causing the error that I mentioned initially.

For example, when I try to query the /api/v1/targets endpoint in Prometheus the response contains a target that has this map of discovered labels:

"discoveredLabels": {
    "__address__": "localhost:19100",
    "__metrics_path__": "/metrics",
    "__param_collect[]": "arp",
    "__scheme__": "http",
    "host": "localhost",
    "job": "node",
}

@krasi-georgiev
Copy link
Contributor

yep I was able to reproduce and your PR fixes the issue.

We started a discussion to see if we can improve these tests by using the actual Prom api module and some sample data instead of mocking the responses.
#528

@krasi-georgiev krasi-georgiev merged commit d5f6310 into prometheus:master Jan 23, 2019
@okushchenko okushchenko deleted the ok/fix-target-labels-type branch January 24, 2019 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants