search

sapcc / ntp_exporter

Prometheus exporter for NTP offset/stratum of a client
Apache License 2.0
50 stars 17 forks source link

Changed mode-2 parameter 'server' to 'target' #14

Closed elcomtik closed 4 years ago

elcomtik commented 4 years ago

According to "MULTI-TARGET EXPORTER PATTERN" (https://prometheus.io/docs/guides/multi-target-exporter/) we should use parameter target to be able to use scrape config like Blackbox exporter use. Currently, we have a parameter server, which has the same purpose. I renamed it to target. Now it is possible to have standard scrape config for multi-target probing.

- job_name: ntp
  metrics_path: /probe
  params:
    protocol: [4]
    duration: [10s]
  static_configs:
    - targets:
      - a.b.c.d
      - e.f.g.h

However, this change is backward incompatible, so exporter version should be bumped to 2.x.x to follow semantic naming convention

majewsky commented 4 years ago

This change could be made backwards-compatible by recognizing both the server and target query parameter and emitting metrics for the servers mentioned in both parameters.

elcomtik commented 4 years ago

@majewsky could you suggest changes by reviewing my commit? I have very little experience with Go language, so I'm not sure how to write it backward compatible

jnovack commented 4 years ago

You don't need to make it backwards compatible. That can be "solved" by just increasing the semver version of the package, that's exactly what it's for, and it's exactly why Go, Node, and large projects adopt semver. An increase of the version from v1.0.0 to v2.0.0 is meant to accommodate breaking changes such as this.

You forgo code complexity for simplicity. It is the downstream user's responsibility to ensure breaking changes are dealt with appropriately.

elcomtik commented 4 years ago

You don't need to make it backwards compatible. That can be "solved" by just increasing the semver version of the package, that's exactly what it's for, and it's exactly why Go, Node, and large projects adopt semver. An increase of the version from v1.0.0 to v2.0.0 is meant to accommodate breaking changes such as this.

You forgo code complexity for simplicity. It is the downstream user's responsibility to ensure breaking changes are dealt with appropriately.

@jnovack If that was my choice I would choose an increase of version. I labeled my internal build to 2.0.0. I don't need to deal with changes, so it is an easy choice for me.

I personally just started to use this exporter, because node exporter wasn't able to probe external NTP servers. This exporter promised that it is capable of it. Actually, if there was a working example of multi-target scrape config using the server parameter, I wouldn't make this code changes. I think, that currently this dynamic probing feature cannot be used according to "MULTI-TARGET EXPORTER PATTERN", so I would trade this feature for hassles with necessary downstream changes.

I let the decision about version change to the project owner to make the final decision. @majewsky

jnovack commented 4 years ago

I'm with you, I'm on your side, my comment was directed towards the project owners to adopt the correction towards prometheus best-practices.

majewsky commented 4 years ago

Yeah, let's go with just renaming server to target and bumping to 2.0

elcomtik commented 4 years ago

@majewsky could you please merge it