docker / go-metrics

Package for metrics collection in Docker projects
Apache License 2.0
87 stars 32 forks source link

Expose the Gatherer and use new registry over prometheus default registry #7

Open jhorwit2 opened 8 years ago

jhorwit2 commented 8 years ago

closes #6

This PR provides a means to get the Registerer and Gatherer being used by the package. It also eliminates using the default prometheus registry. This means that things like the GC handler included in the default registry won't be exported on /metrics or Gather(). This gives support for the package to include it optionally (say debug mode in docker).

Signed-off-by: Josh Horwitz horwitzja@gmail.com

jhorwit2 commented 8 years ago

cc @crosbymichael

jhorwit2 commented 8 years ago

@crosbymichael I removed Registerer(); however, technically because registry is both a Registerer and Gatherer you could cast between the two via Gatherer(). Gatherer() is useful because you can use it to combine multiple registries for exporting metrics.

crosbymichael commented 8 years ago

ping @stevvooe Can you take a look plz?

stevvooe commented 8 years ago

I see the use case of wanting to only reference the metrics coming through this package but I'd still like to register everything with the default prometheus registry. To tell you the trust, centralizing on a global in this package seems problematic. This package should mostly be about declaration and routing.

I do see that the tests are much easier. If you look at some of my earlier tests in contribs to the prom repo, I had to decode to protos to test anything.

@jhorwit2 What if we made the namespace implement Gatherer? Would that address the original goal?

crosbymichael commented 8 years ago

@stevvooe why do you think we need to use the global prom registry. One of the problems what we have using it is that things like etcd metrics are automatically registered and show up in our output because someone imports an etcd package in our codebase.

If we have our own instance of the registry we we can only output docker metrics

jhorwit2 commented 8 years ago

@stevvooe Yea, having namespace implement the Gatherer would work 👍

crosbymichael commented 8 years ago

What i want to do is this. Have a global registry in this package that works behind the metrics.Register commands so that all docker projects can create and register their metrics and they are displayed via:

/metrics

Then I want a sub registry and handler for container level metrics to replace docker stats that only shows container metrics at:

/containers/metrics

Being a new handler and a separate registry.

jhorwit2 commented 8 years ago

@crosbymichael if that's what you want then it seems each namespace needs its own registry and to implement Gatherer

stevvooe commented 8 years ago

@stevvooe why do you think we need to use the global prom registry.

I just want to be a team player.

I think the plan of having a central registry here is sufficient. My only concern would be that it may be complicated for docker to control the assembly of that registry. It may be better to have each upstream project export a registry and then assemble that in docker.

crosbymichael commented 8 years ago

@stevvooe yes ,i mean keep a central registry but don't use the prom packages one so that etcd and other metrics are added to it when we don't use it.

Keep the Register method in this package the entrypoint for all upstream projects

jhorwit2 commented 8 years ago

I think I have an idea that will make both of you happy 👼 . I'll update this PR.

stevvooe commented 6 years ago

@jhorwit2 Are you planning to update this PR?

dmp42 commented 6 years ago

@jhorwit2 any chance you could get this one back on track?