Wiser coders than myself have come to the conclusion that a `switch`
statement is almost always superior to a statement that includes any
`else if`.
The exceptions that I have found in our codebase are just these two:
* The `if else` is followed by an additional statement before the next
condition (separated by a `;`).
* The whole thing is within a `for` loop and `break` statements are
used. In this case, using `switch` would require tagging the `for`
loop, which probably tips the balance.
Why are `switch` statements more readable?
For one, fewer curly braces. But more importantly, the conditions all
have the same alignment, so the whole thing follows the natural flow
of going down a list of conditions. With `else if`, in contrast, all
conditions but the first are "hidden" behind `} else if `, harder to
spot and (for no good reason) presented differently from the first
condition.
I'm sure the aforemention wise coders can list even more reasons.
In any case, I like it so much that I have found myself recommending
it in code reviews. I would like to make it a habit in our code base,
without making it a hard requirement that we would test on the CI. But
for that, there has to be a role model, so this commit eliminates all
`if else` occurrences, unless it is autogenerated code or fits one of
the exceptions above.
Signed-off-by: beorn7 <beorn@grafana.com>
We haven't updated golint-ci in our CI yet, but this commit prepares
for that.
There are a lot of new warnings, and it is mostly because the "revive"
linter got updated. I agree with most of the new warnings, mostly
around not naming unused function parameters (although it is justified
in some cases for documentation purposes – while things like mocks are
a good example where not naming the parameter is clearer).
I'm pretty upset about the "empty block" warning to include `for`
loops. It's such a common pattern to do something in the head of the
`for` loop and then have an empty block. There is still an open issue
about this: https://github.com/mgechev/revive/issues/810 I have
disabled "revive" altogether in files where empty blocks are used
excessively, and I have made the effort to add individual
`// nolint:revive` where empty blocks are used just once or twice.
It's borderline noisy, though, but let's go with it for now.
I should mention that none of the "empty block" warnings for `for`
loop bodies were legitimate.
Signed-off-by: beorn7 <beorn@grafana.com>
This makes it easier to connect a log message with the config it relates
to.
Each SD config has a name, either the scrape job name or something like
"config-0" for Alertmanager config.
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
While originally the resync period also forced refreshing from Kubernetes API server, this has been removed for some years now because watching the API server got more stable [1]. Today, this just results in all entities being sent to the service discovery again, which is valid from a general Prometheus perspective, but results in unnecessary CPU load and also breaks service discovery metrics. In especially, this makes monitoring "do we actually observe changes from Kubernetes API server" impossible (receiving constant updates from Kubernetes service discovery is a pretty valid assumption, for example nodes get frequent status updates, ...).
Signed-off-by: Jens Erat <jens.erat@mercedes-benz.com>
* Add VM size label to azure service discovery (#11575)
Signed-off-by: davidifr <davidfr.mail@gmail.com>
* Add VM size label to azure service discovery (#11575)
Signed-off-by: davidifr <davidfr.mail@gmail.com>
* Add VM size label to azure service discovery (#11575)
Signed-off-by: davidifr <davidfr.mail@gmail.com>
Signed-off-by: davidifr <davidfr.mail@gmail.com>
A new API is available for AddEventHandlers, to get errors but also be
able to cancel handlers.
Doing the easy thing for the release, which is just to log errors.
We could see how to improve this in the future to handle the errors
properly and cancel the handlers.
Signed-off-by: Julien Pivotto <roidelapluie@o11y.eu>
* Common client in EC2 and Lightsail
Signed-off-by: Levi Harrison <git@leviharrison.dev>
* Azure -> AWS
Signed-off-by: Levi Harrison <git@leviharrison.dev>
Signed-off-by: Levi Harrison <git@leviharrison.dev>
* file sd: create and increment an inotify error counter when file-SD is unable to watch files. Additionally, order metrics alphabetically.
Signed-off-by: Michael Fuller <mfuller@digitalocean.com>
* file.go: consistent naming and help for prometheus_sd_file_watcher_errors_total
Signed-off-by: Michael Fuller <mfuller@digitalocean.com>
Signed-off-by: Michael Fuller <mfuller@digitalocean.com>
Co-authored-by: Michael Fuller <mfuller@digitalocean.com>
Function arguments in defer evaluated during definition of defer, not
during execution
Signed-off-by: Slavik Panasovets <slavik@google.com>
Signed-off-by: Slavik Panasovets <slavik@google.com>
* Update go to 1.19, set min version to 1.18
Signed-off-by: Julien Pivotto <roidelapluie@o11y.eu>
* Update golangci-lint
Signed-off-by: Julien Pivotto <roidelapluie@o11y.eu>
Signed-off-by: Julien Pivotto <roidelapluie@o11y.eu>
This fixes an occurrence of a loop variable being captured in a
parallel test (`TestInitialUpdate`). Prior to this commit, only the
last test case declared in that test would actually execute. To work
around this problem, we create a copy of the range variable before the
paralllel test, as suggested in the documentation for the `testing`
package:
https://pkg.go.dev/testing#hdr-Subtests_and_Sub_benchmarks
The test immediately after the one fixed here (`TestInvalidFile`)
followed the same pattern but correctly created a copy of the loop
variable, illustrating how easy it is to forget to do this in
practice.
Issue was automatically found using the `loopvarcapture` linter.
Signed-off-by: Renato Costa <renato@cockroachlabs.com>
Signed-off-by: Renato Costa <renato@cockroachlabs.com>
It's currently possible to use blackbox_exporter to probe MX records
themselves. However it's not possible to do an end-to-end test, like is
possible with SRV records. This makes it possible to use MX records as a
source of hostnames in the same way as SRV records.
Signed-off-by: David Leadbeater <dgl@dgl.cx>
This commits adds a __meta_kubernetes_pod_container_image as a new
metadata label. This can be used to alert on mismatched versions of
targets who don't have a build_info metric, as well as injecting it into
log lines for other consumers of discovery/kubernetes (e.g., Promtail).
Signed-off-by: Robert Fratto <robertfratto@gmail.com>
The Kubernetes service discovery can only add node labels to
targets from the pod role.
This commit extends this functionality to the endpoints and
endpointslices roles.
Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>
The tests for Kubernetes SD rely on comparing target groups by first
serializing them to JSON. However, the target group MarshalJSON function
only serializes the __address__ label, which makes eliminates all other
labels from the comparison.
This commit implements a separate marshaling function intended for use in
Kubernetes SD tests. The function serializes all target labels, making
comparisons much more reliable. The commit also fixes all tests that
started to fail due to the newly introduced change.
Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>
This commit introduces a new metric to count the number of failed
requests to Linode's API when using Linode SD. Resolves#10672, inspired
by #10476.
_Note_: this doens't count failures when polling the `/account/events`
endpoint, as a `401` there is how we determine if the supplied token has
the needed API scopes to do event polling vs full refreshes each
interval.
Signed-off-by: TJ Hoplock <t.hoplock@gmail.com>
