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

Add Prometheus metrics for number of log messages #65

Merged
merged 3 commits into from
Oct 4, 2017

Conversation

marccarre
Copy link
Contributor

@marccarre marccarre commented Oct 3, 2017

Promrus is a Logrus hook to expose the number of log messages as Prometheus metrics:

log_messages{level="debug"}
log_messages{level="info"}
log_messages{level="warning"}
log_messages{level="error"}

Added dependency using:

$ dep ensure github.com/weaveworks/promrus@v1.0.0 && dep ensure && dep prune

Fixed casing in logrus imports, causing the following issue:

middleware/grpc_logging.go:10:2: case-insensitive import collision:
"github.com/weaveworks/common/vendor/github.com/Sirupsen/logrus" and
"github.com/weaveworks/common/vendor/github.com/sirupsen/logrus"

by:

  • changing import statements from Sirupsen/logrus to sirupsen/logrus, as per the official documentation, and
  • running dep ensure && dep prune.

Added dependency using:
```
$ dep ensure github.com/marccarre/promrus@v1.0.0 && dep ensure && dep prune
```
This fixes:
```
middleware/grpc_logging.go:10:2: case-insensitive import collision: "github.com/weaveworks/common/vendor/github.com/Sirupsen/logrus" and "github.com/weaveworks/common/vendor/github.com/sirupsen/logrus"
```
Changelog:
- Changed `import` statements from `Sirupsen/logrus` to `sirupsen/logrus`, as per the official documentation: https://github.com/sirupsen/logrus
- Ran:
```
dep ensure && dep prune
```
@marccarre marccarre requested a review from jml October 3, 2017 15:05
Copy link
Contributor

@jml jml left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sure about the Sirupsen case thing. I got bit by it a while ago. I suggest trying to use this branch of common in a project with executables before merging it.

@marccarre
Copy link
Contributor Author

What do you mean by not sure?
Do you think it could cause issues to change it?
Is there some sort of a convention I missed regarding this?

I initially did not realise this would lead to such changes when I added sirupsen/logrus to my dependencies in promrus by simply following their documentation. I then realised there was this casing difference here, but also just saw, for example, that Cortex's dependencies are also using Sirupsen/logrus, so if I keep promrus as is, I believe it might lead to more build issues, case changes (I don't mind making these changes if they are painless), or even preventing metrics from being exposed since Go might treat them as different packages altogether (?).
Alternatively, I could just change promrus to use Sirupsen/logrus instead of sirupsen/logrus.

@marccarre
Copy link
Contributor Author

In any case, I'll try it against Cortex and will report back.

@jml
Copy link
Contributor

jml commented Oct 3, 2017

Not sure as in "I did something like this a few months ago and regretted it but can't recall the details". It might well break things.

@marccarre
Copy link
Contributor Author

FYI, looks like we aren't the only ones facing this issue, see here and here, and that the way forward is to use lowercase. I'll bite the bullet and make the necessary changes where we decide to use promrus.

@marccarre
Copy link
Contributor Author

marccarre commented Oct 4, 2017

I haven't tried on Cortex yet, but trying on a reduction of the problem (i.e. a smaller project mimicking its dependencies) shows that:

  1. having mixed casing in vendor definitely breaks the build,
  2. even if you only have sirupsen/logrus in vendor (e.g.: dep ensure and then rm -fr vendor/github.com/Sirupsen), instrumenting on sirupsen/logrus does not instrument Sirupsen/logrus:

foo.go in project foo:

package foo

import (
	log "github.com/Sirupsen/logrus"
)

// Sirupsen sirupsens.
func Sirupsen() {
	log.Infof("Sirupsen/logrus")
}

main.go in a different project:

package main

import (
	"net/http"
	"time"

	"github.com/marccarre/promrus"
	"github.com/marccarre/foo"
	"github.com/prometheus/client_golang/prometheus/promhttp"
	log "github.com/sirupsen/logrus"
)

func main() {
	hook, err := promrus.NewPrometheusHook()
	if err != nil {
		return
	}
	log.AddHook(hook)
	go http.ListenAndServe(":8080", promhttp.Handler())
	for {
		foo.Sirupsen()
		time.Sleep(10 * time.Second)
	}
}

yields metrics which do not increase over time:

$ curl -fsS localhost:8080 | grep log_messages
# HELP log_messages Total number of log messages.
# TYPE log_messages counter
log_messages{level="debug"} 0
log_messages{level="error"} 0
log_messages{level="info"} 0
log_messages{level="warning"} 0

The `promrus` project's ownership has changed, and now belongs to Weaveworks.
Command ran:
```
$ dep ensure github.com/weaveworks/promrus@v1.0.0 && dep ensure && dep prune
```
@marccarre marccarre merged commit 79ec4e6 into master Oct 4, 2017
@marccarre marccarre deleted the add-prom-metrics branch October 4, 2017 15:28
@bboreham
Copy link
Collaborator

bboreham commented Oct 6, 2017

Why do we even want this?

@marccarre
Copy link
Contributor Author

marccarre commented Oct 6, 2017

What do you mean by "this"?

Feel free to revert if causing issues though.

@@ -27,6 +28,11 @@ func Setup(logLevel string) error {
}
log.SetLevel(level)
log.SetFormatter(&textFormatter{})
hook, err := promrus.NewPrometheusHook() // Expose number of log messages as Prometheus metrics.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

yeya24 pushed a commit to yeya24/common that referenced this pull request Jun 12, 2024
Adjust the MetricNameRE to reflect the actual behavior
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.

3 participants