Add PuppetDB service discovery by roidelapluie · Pull Request #8883 · prometheus/prometheus

roidelapluie · 2021-06-01T20:03:30Z

We have been Puppet user for 10 years and we are users of
https://github.com/camptocamp/prometheus-puppetdb-sd

However, that file_sd implementation contains business logic and
assumptions around e.g. the modules which you are using.

This pull request adds a simple PuppetDB service discovery, which will
enable more use cases than the mentioned sd.

cc @raphink @mcanevet

Signed-off-by: Julien Pivotto roidelapluie@inuits.eu

roidelapluie · 2021-06-09T14:41:28Z

We are using this in production.

@mcanevet would you have some time to try this out as well?

mcanevet · 2021-06-09T14:47:30Z

@roidelapluie do you have a pre-built docker image with this patch so that I don't have to build one?

roidelapluie · 2021-06-16T11:54:50Z

docker pull quay.io/roidelapluie/prometheus-linux-amd64:puppetdb-sd

saimonn · 2021-07-23T13:19:52Z

@roidelapluie Thanks for this PR.
I've tested your image in our environment, and it works as expected.

NB: I just had to remove the suffix /pdb/api/v4 from the url, as it seems to already be added here. Should the test (also here and in other places) be adjusted accordingly ?

roidelapluie · 2021-09-01T10:37:20Z

This is ready for review, and tests are green.

juliusv · 2021-09-01T11:39:37Z

@roidelapluie Hi, I'm on vacation starting tomorrow until Sept 14 and won't be able to get to this before then. But happy to take a look after!

roidelapluie · 2021-09-01T11:57:54Z

@roidelapluie Hi, I'm on vacation starting tomorrow until Sept 14 and won't be able to get to this before then. But happy to take a look after!

Enjoy!

roidelapluie · 2021-09-15T11:13:39Z

@roidelapluie Hi, I'm on vacation starting tomorrow until Sept 14 and won't be able to get to this before then. But happy to take a look after!

Friendly ping :)

juliusv

Looks great! Just a couple of nits :)

juliusv · 2021-09-16T10:15:18Z

discovery/puppetdb/puppetdb.go

+		return err
+	}
+	if c.URL == "" {
+		return fmt.Errorf("url missing")


To make it consistent with file_sd:

Suggested change

return fmt.Errorf("url missing")

return fmt.Errorf("URL is missing")

juliusv · 2021-09-16T10:15:47Z

discovery/puppetdb/puppetdb.go

+		return err
+	}
+	if parsedURL.Scheme != "http" && parsedURL.Scheme != "https" {
+		return fmt.Errorf("url scheme must be http or https")


Same:

Suggested change

return fmt.Errorf("url scheme must be http or https")

return fmt.Errorf("URL scheme must be 'http' or 'https'")

juliusv · 2021-09-16T10:57:16Z

discovery/puppetdb/resources.go

+		case string:
+			labelValue = value
+		case bool:
+			labelValue = fmt.Sprintf("%t", value)


Btw., you can use strconv.FormatBool() here:

Suggested change

labelValue = fmt.Sprintf("%t", value)

labelValue = strconv.FormatBool(value)

juliusv · 2021-09-16T10:58:47Z

discovery/puppetdb/resources.go

+				case string:
+					values[i] = value
+				case bool:
+					values[i] = fmt.Sprintf("%t", value)


juliusv · 2021-09-16T11:04:54Z

docs/configuration/configuration.md

+* `__meta_puppetdb_resource`: a SHA-1 hash of the resource’s type, title, and parameters, for identification
+* `__meta_puppetdb_type`: the resource type
+* `__meta_puppetdb_title`: the resource title
+* `__meta_puppetdb_exported`: whether or not the resource is exported (true, false)


Suggested change

* `__meta_puppetdb_exported`: whether or not the resource is exported (true, false)

* `__meta_puppetdb_exported`: whether or not the resource is exported (`"true"` or `"false"`)

juliusv · 2021-09-16T11:06:23Z

docs/configuration/configuration.md

+* `__meta_puppetdb_exported`: whether or not the resource is exported (true, false)
+* `__meta_puppetdb_tags`: comma separated list of resource tags
+* `__meta_puppetdb_file`: the manifest file in which the resource was declared
+* `__meta_puppetdb_environment`: the environment of the node associated to the resource


"associated with"?

juliusv · 2021-09-16T11:06:43Z

docs/configuration/configuration.md

+* `__meta_puppetdb_tags`: comma separated list of resource tags
+* `__meta_puppetdb_file`: the manifest file in which the resource was declared
+* `__meta_puppetdb_environment`: the environment of the node associated to the resource
+* `__meta_puppetdb_parameter_<parametername>`: the parameters of the resouces


Suggested change

* `__meta_puppetdb_parameter_<parametername>`: the parameters of the resouces

* `__meta_puppetdb_parameter_<parametername>`: the parameters of the resources

juliusv · 2021-09-16T11:08:36Z

docs/configuration/configuration.md

+# https://puppet.com/docs/puppetdb/latest/api/query/v4/pql.html
+query: <string>
+
+# Whether to include the parameters as labels.


To make it even clearer:

Suggested change

# Whether to include the parameters as labels.

# Whether to include the parameters as meta labels.

roidelapluie · 2021-09-16T14:49:42Z

I have addressed the comments

juliusv · 2021-09-16T14:51:10Z

@roidelapluie could it be that you need to rebase on the latest main to make the Netflify checks work?

juliusv · 2021-09-16T14:51:26Z

Other than that, 👍

juliusv · 2021-09-16T14:51:55Z

...but IMO it'd also be ok to merge it with Netlify failing, since it doesn't touch anything UI-related.

We have been Puppet user for 10 years and we are users of https://github.com/camptocamp/prometheus-puppetdb-sd However, that file_sd implementation contains business logic and assumptions around e.g. the modules which you are using. This pull request adds a simple PuppetDB service discovery, which will enable more use cases than the upstream sd. Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>

roidelapluie · 2021-09-16T14:54:51Z

Rebased & squashed :)

juliusv · 2021-09-16T15:41:18Z

Hmm, now the Netlify deploy got cancelled with Canceled build due to no content change.

I think it's fine though. Merging :)

roidelapluie marked this pull request as draft June 1, 2021 20:13

roidelapluie marked this pull request as ready for review June 9, 2021 14:41

roidelapluie force-pushed the puppetdb-sd branch from ef3520f to 19cee55 Compare July 27, 2021 12:43

roidelapluie force-pushed the puppetdb-sd branch from 19cee55 to 49ade02 Compare August 31, 2021 12:02

roidelapluie requested a review from juliusv August 31, 2021 12:04

roidelapluie force-pushed the puppetdb-sd branch from 49ade02 to e23a26b Compare September 1, 2021 09:48

juliusv reviewed Sep 16, 2021

View reviewed changes

roidelapluie force-pushed the puppetdb-sd branch from d101d5a to 8920024 Compare September 16, 2021 14:54

juliusv merged commit 62906c1 into prometheus:main Sep 16, 2021

ptodev mentioned this pull request Jan 16, 2024

SD Managers taking over responsibility for registration of debug metrics #13375

Merged

ptodev mentioned this pull request Jan 23, 2024

Change metric label for Puppet SD from 'http' to 'puppetdb' #13447

Merged

	return fmt.Errorf("url missing")
	return fmt.Errorf("URL is missing")

	return fmt.Errorf("url scheme must be http or https")
	return fmt.Errorf("URL scheme must be 'http' or 'https'")

	labelValue = fmt.Sprintf("%t", value)
	labelValue = strconv.FormatBool(value)

	* `__meta_puppetdb_exported`: whether or not the resource is exported (true, false)
	* `__meta_puppetdb_exported`: whether or not the resource is exported (`"true"` or `"false"`)

	* `__meta_puppetdb_parameter_<parametername>`: the parameters of the resouces
	* `__meta_puppetdb_parameter_<parametername>`: the parameters of the resources

	# Whether to include the parameters as labels.
	# Whether to include the parameters as meta labels.

Conversation

roidelapluie commented Jun 1, 2021

Uh oh!

roidelapluie commented Jun 9, 2021

Uh oh!

mcanevet commented Jun 9, 2021

Uh oh!

roidelapluie commented Jun 16, 2021

Uh oh!

saimonn commented Jul 23, 2021

Uh oh!

roidelapluie commented Sep 1, 2021

Uh oh!

juliusv commented Sep 1, 2021

Uh oh!

roidelapluie commented Sep 1, 2021

Uh oh!

roidelapluie commented Sep 15, 2021

Uh oh!

juliusv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roidelapluie commented Sep 16, 2021

Uh oh!

juliusv commented Sep 16, 2021

Uh oh!

juliusv commented Sep 16, 2021

Uh oh!

juliusv commented Sep 16, 2021

Uh oh!

roidelapluie commented Sep 16, 2021

Uh oh!

juliusv commented Sep 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants