Add new collectors package #862

johejo · 2021-04-24T18:10:02Z

Signed-off-by: Mitsuo Heijo [email protected]

https://groups.google.com/g/prometheus-developers/c/Zexpd_oRYRQ
#848 (comment)

Based on the above discussion, we have added a new collector package.
The destination collectors are now a wrapper for the previous package to avoid circular imports.
I'm not sure if this is a better change.
It seems to me that this changes should be done for v2, not v1.

beorn7 · 2021-04-27T21:27:30Z

This is a good change to do before v2. In v2, we'll remove the now deprecated functions. By having the new functions in v1 already, people can gradually migrate over so that it doesn't have to happen as a big bang precisely on upgrading to v2.

I would call the package collectors, not collector. (Otherwise, I'd assume the Collector interface is in the collector package, but that's not the case.)

You also have to disable the lint warnings, see https://github.com/prometheus/client_golang/blob/master/prometheus/promhttp/delegator.go#L86 to get an idea how that works.

beorn7

Looks good in general, thank you very much. Just some nits about doc comments, see below.

Also, there is an example for NewExpvarCollector, which should be moved into the new package, too.

beorn7 · 2021-04-29T20:00:37Z

prometheus/collectors/collectors.go

+// limitations under the License.
+
+// Package collectors provides several collectors that implement
+// prometheus.Collector.


That's a bit of a stutter and doesn't say so much. How about the following:

Package collectors provides implementations of prometheus.Collector to conveniently collect process and Go-related metrics.

beorn7 · 2021-04-29T20:02:11Z

prometheus/collectors/expvar_collector.go

+import "github.com/prometheus/client_golang/prometheus"
+
+// NewExpvarCollector returns a newly allocated expvar Collector that still has
+// to be registered with a Prometheus registry.


The part "that still has to be registered with a Prometheus registry" comes from the days when this was the only collector provided by the library. It has become confusing by now (because it's also true for the other collectors, but we don't say so in their doc comments). Now that it is even in its own package, we should really delete that part.

beorn7 · 2021-04-29T20:03:36Z

prometheus/collectors/expvar_collector.go

+// Currently this function is a wrapper for prometheus.NewExpvarCollector to
+// prevent cycle imports.


That's an implementation detail without any impact for the users of the library. I would not mention it in the doc comment.

beorn7 · 2021-04-29T20:05:56Z

prometheus/collectors/go_collector.go

+// Currently this function is a wrapper for prometheus.NewGoCollector to
+// prevent cycle imports.


Please remove, see above.

beorn7 · 2021-04-29T20:06:09Z

prometheus/collectors/go_collector.go

+// Currently this function is a wrapper for prometheus.NewGoCollector to
+// prevent cycle imports.


Please remove, see above.

beorn7 · 2021-04-29T20:06:43Z

prometheus/collectors/process_collector.go

+// Currently this function is a wrapper for prometheus.NewProcessCollector to
+// prevent cycle imports.


Please remove, see above.

beorn7 · 2021-04-29T20:11:06Z

prometheus/expvar_collector.go

@@ -59,6 +59,8 @@ type expvarCollector struct {
 // sample values.
 //
 // Anything that does not fit into the scheme above is silently ignored.
+//
+// Deprecated: Use collectors.NewExpvarCollector instead.


Perhaps it's best to also remove the whole lengthy doc comment above, e.g.:

// NewExpvarCollector is the obsolete version of collectors.NewExpvarCollector. // See there for documentation. // // Deprecated: Use collectors.NewExpvarCollector instead.

The last line might seem redundant now, but it's important to have it in this format for linters.

Same for the doc comments for the other, now deprecated, functions.

johejo · 2021-04-30T12:30:42Z

Thanks for the review.
Another question, how should ProcessCollectorOpts be migrated?
There are also other option such as type alias.

beorn7 · 2021-05-06T12:21:08Z

Sorry for the delay. This is on my list. I hope I get to this very soon.

beorn7

One minor thing left.

beorn7 · 2021-05-06T17:34:32Z

prometheus/go_collector.go

@@ -383,6 +361,8 @@ type memStatsMetrics []struct {
 // This collector uses only the build information for the main module. See
 // https://github.com/povilasv/prommod for an example of a collector for the
 // module dependencies.
+//


The doc comment above also needs to be replaced with

// NewBuildInfoCollector is the obsolete version of collectors.NewGoCollector. // See there for documentation.

beorn7 · 2021-05-06T17:39:18Z

Another question, how should ProcessCollectorOpts be migrated?
There are also other option such as type alias.

I guess, in general, a type alias is a good idea. However, in this case, we needed to put the alias in the collectors package, again to avoid a circular import chain. So the "real" type with all the doc comments would be in the location where it is deprecated.

I'm also not sure if a type alias would help that much in this particular case. If you use prometheus.NewProcessCollector, you also use prometheus.ProcessCollectorOpts. If you use collectors.NewProcessCollector, you use collectors.ProcessCollectorOpts. So at the time you change the one, you can also change the other.

Signed-off-by: Mitsuo Heijo <[email protected]>

beorn7

Thank you very much.

And now on to adding your actual new code. (o:

beorn7 self-requested a review April 26, 2021 21:52

johejo force-pushed the new_collector_package branch 2 times, most recently from 133bd61 to d938323 Compare April 29, 2021 06:13

johejo marked this pull request as ready for review April 29, 2021 06:42

beorn7 requested changes Apr 29, 2021

View reviewed changes

johejo force-pushed the new_collector_package branch from d938323 to 24dc2a6 Compare April 30, 2021 13:02

johejo changed the title ~~Add new collector package~~ Add new collectors package Apr 30, 2021

johejo force-pushed the new_collector_package branch from 24dc2a6 to 6f283bd Compare April 30, 2021 13:03

johejo requested a review from beorn7 May 6, 2021 11:38

beorn7 requested changes May 6, 2021

View reviewed changes

johejo force-pushed the new_collector_package branch 4 times, most recently from 80510b9 to 2931a50 Compare May 8, 2021 01:12

Add new collectors package

e07445a

Signed-off-by: Mitsuo Heijo <[email protected]>

johejo force-pushed the new_collector_package branch from 2931a50 to e07445a Compare May 8, 2021 01:43

johejo requested a review from beorn7 May 8, 2021 01:55

beorn7 approved these changes May 10, 2021

View reviewed changes

beorn7 merged commit b89620c into prometheus:master May 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new collectors package #862

Add new collectors package #862

johejo commented Apr 24, 2021

beorn7 commented Apr 27, 2021

beorn7 left a comment

beorn7 Apr 29, 2021

beorn7 Apr 29, 2021

beorn7 Apr 29, 2021

beorn7 Apr 29, 2021

beorn7 Apr 29, 2021

beorn7 Apr 29, 2021

beorn7 Apr 29, 2021

johejo commented Apr 30, 2021

beorn7 commented May 6, 2021

beorn7 left a comment

beorn7 May 6, 2021

beorn7 commented May 6, 2021

beorn7 left a comment

		// Currently this function is a wrapper for prometheus.NewExpvarCollector to
		// prevent cycle imports.

Add new collectors package #862

Add new collectors package #862

Conversation

johejo commented Apr 24, 2021

beorn7 commented Apr 27, 2021

beorn7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johejo commented Apr 30, 2021

beorn7 commented May 6, 2021

beorn7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beorn7 commented May 6, 2021

beorn7 left a comment

Choose a reason for hiding this comment