Adding prometheus and pushgateway outputs #230
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
👋 Thanks for submitting your first PR!
Please be sure to read and follow our Code of Conduct and Contributing guide.
⭐️ Is your org or open source project using woke? If so, we'd love for you to be included in the 'Who uses woke' list at https://github.com/get-woke/woke/blob/main/docs/about.md#who-uses-woke.
pkg/config/config.go
Outdated
| Labels map[string]string `yaml:"labels,omitempty"` | ||
| PushgatewayURL string `yaml:"pushgateway"` |
There was a problem hiding this comment.
As discussed over email, please don't put these output-specific configs at the top level of woke config. I would prefer something else like
outputs:
prometheus:
pushgateway: https://my.url/
labels:
repo: sample
something: else| return &Pushgateway{ | ||
| writer: w, | ||
| labels: c.Labels, | ||
| pushgatewayURL: c.PushgatewayURL, |
There was a problem hiding this comment.
Do you need to validate that PushgatewayURL is supplied so you can provide an error to the user?
| if err := pusher.Push(); err != nil { | ||
| fmt.Println("Could not push woke_result to Pushgateway:", err) | ||
| } else { | ||
| fmt.Printf("push woke_result to Pushgateway: %v", pusher) |
There was a problem hiding this comment.
Can you use the zerolog log package for this instead of fmt to print something that's meant to be informative?
Also, using %v here doesn't provide a lot of valuable information to the user, what are you trying to tell the user here by printing out the pusher instead of the result from woke? That data can easily be written out in a human-readable format
| Collector(woke_result) | ||
|
|
||
| if err := pusher.Push(); err != nil { | ||
| fmt.Println("Could not push woke_result to Pushgateway:", err) |
There was a problem hiding this comment.
Should this error be returned? If not, the log package would be preferred to fmt here too
| if len(t.labels) > 0 { | ||
| first := true | ||
| for k, v := range t.labels { | ||
| if first { | ||
| labelString += ", " | ||
| first = false | ||
| } | ||
| labelString += k + "=" + v + ", " | ||
| } | ||
| labelString = labelString[:len(labelString)-2] | ||
| } |
There was a problem hiding this comment.
This logic is duplicated across prometheus.go and pushgateway.go. Can they be consolidated? I would also recommend creating a test for this functionality specifically
| Name: "woke_result", | ||
| Help: "Inclusive woke result", | ||
| }) | ||
| pusher := push.New(t.pushgatewayURL, "woke") |
There was a problem hiding this comment.
Is there a reason not to initialize this before looping through the results?
| @@ -0,0 +1,85 @@ | |||
| /** | |||
There was a problem hiding this comment.
Please include exhaustive tests for this file.
pkg/printer/printer.go
Outdated
| @@ -67,6 +76,10 @@ func NewPrinter(f string, w io.Writer) (Printer, error) { | |||
| p = NewJSON(w) | |||
| case OutFormatSonarQube: | |||
| p = NewSonarQube(w) | |||
| case OutFormatPrometheus: | |||
| p = NewPrometheus(w, c) | |||
There was a problem hiding this comment.
Ideally you do not need to pass the entire config into these two New* functions, you should only need to pass in what is needed for the printer to run.
pkg/printer/printer.go
Outdated
| // OutFormatPrometheus outputs in prometheus format | ||
| OutFormatPrometheus = "prometheus" | ||
|
|
||
| // OutFormatPushgateway send metrics to prometheus pushgateway | ||
| OutFormatPushgateway = "pushgateway" |
There was a problem hiding this comment.
These are both prometheus output formats? Why do they need to be two unique ones? Also, please include links here to prometheus format like the other formats provide
go.mod
Outdated
| golang.org/x/text v0.3.7 // indirect | ||
| gopkg.in/ini.v1 v1.66.4 // indirect | ||
| google.golang.org/protobuf v1.28.0 // indirect | ||
| gopkg.in/ini.v1 v1.67.0 // indirect |
There was a problem hiding this comment.
There are a lot of version upgrades here that I'm not sure are relevant to this PR.
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make due to this PR?)
Other information: