iits-consulting / otc-prometheus-exporter

GNU General Public License v3.0
8 stars 1 forks source link

Feature: Add zap logging and fix some small issues #38

Closed DrDJIng closed 9 months ago

DrDJIng commented 10 months ago

Hello again!

I've been debugging some issues I'm having with the new version of the exporter, and needed to add logging in. I decided to use zap, which I hope is appropriate.

I've included a bunch of logging already, and due to the use of zap, it should be cluster-friendly logging, and comes with the added benefit of a json output (which can be changed to yaml if needed).

I also:

Please let me know if I've done anything silly, this is my first larger golang contribution.

Thanks!

DrDJIng commented 10 months ago

I also found that the initial call of metrics wasn't reading all the pages. This seems to be an issue with the default page limit?

In any case, explicitly setting the limit to the maximum allowed retrieved all our metrics, so I added it in here with the other small fixes.

If needed, I can remove the fixes and created another PR specifically for those?

zeljkobekcic commented 10 months ago

Hello!

Your PR is very welcome and the scope of it okay too.

There are some things which I think could be changed. We can talk over them. I will just comment the code in the review function here.

zeljkobekcic commented 10 months ago

I am not opposed to zap as a logging library. But does it do something better except performance than go's included slog https://go.dev/blog/slog ?

Daverinoe commented 10 months ago

I'll be honest and say that when I googled good level and structured go loggers for kubernetes clusters, zap came up and looked easy to use, so that was most of the decision process.

I'm also not opposed to switching to slog to keep the imports down, if that's preferred!

zeljkobekcic commented 10 months ago

That's okay. I just wanted to know if there is something else driving the decision that I am not aware of.

EDIT:

Even if we were to swap out the logger because of reasons, then it would be easy because you are using the interface there.

DrDJIng commented 10 months ago

Whoops, sorry, replied from another computer that was logged into my game dev account!

I've committed the changes you commented on. Thanks for telling me about pushing the logger into the struct, that really seemed to clean up the code!

zeljkobekcic commented 10 months ago

Also, you would need to fix the pipeline. In this particular case I don't see a problem to ignore the error with a comment.

//nolint:errcheck // not relevant should do the trick.

zeljkobekcic commented 10 months ago

@DrDJIng I guess it's alright. When the pipiline builds, then I will merge it.