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

Switch config to proto3 #670

Closed
fabxc opened this Issue May 5, 2015 · 10 comments

Comments

Projects
None yet
4 participants
@fabxc
Copy link
Member

fabxc commented May 5, 2015

The lack of maps in proto2 results in a workaround, looking like this:

labels {
  label {name: "foo" value: "bar"}
  label {name: "bo" value: "om"}
}

As labels are an integral part of Prometheus's config this is not ideal.
With the new service discovery, third party apps may have to emit target groups, which contain labels. So this is no longer a config-only issue and will be harder to break after the SD release.

Support for maps was added in proto3, which is still in alpha. The result looks like this:

label {key:"foo" value:"bar"}
label {key:"bo" value:"om"}

The Go library supports (de)serializing from and to JSON. With proto3 this becomes part of the standard implementation.
While it's not as interesting for the standard config, third party SDs will be more likely to emit JSON targets groups than protocol buffers. In JSON the difference is more striking:

"labels": {
  "label": [
    {"name": "foo", "value": "bar"},
    {"name": "bo", "value":"om"}
  ]
}

versus

"labels": {
   "foo": "bar",
   "bo": "om",
}

Handling the input in Go is more convenient as a map in proto3 results in a map in Go.

Proto3 does not support customized default values, the optional and required keywords were removed. All values default to their types zero value. As a result the generated Go code does not have pointers for scalar values, which is easier to deal with.
We can no longer detect if the user set the zero value explicitly or if it was set by default. For the current config this does not cause a problem for us. For future additions we have to make sure that the zero value is either the default or not valid.

These are the facts affecting us.

Personal opinion:
The alpha status is a non-issue for parsing config files in text format/JSON.
The main advantage lies in the better mapping to Go maps and more intuitive JSON equivalent.
Not having defaults configurable seems negative at first but we already do custom defaulting (global config) in the code anyway - we might just as well do it all manually - it's a one-time effort. Handling all non-syntactic validation/defaulting in our own code adds more value in terms of clarity than it causes pain.

/cc @brian-brazil @beorn7 @juliusv

We are breaking the config either way - so now is the time to break it badly. So any ideas welcome.

Proto3 manual: https://developers.google.com/protocol-buffers/docs/proto3

@beorn7

This comment has been minimized.

Copy link
Member

beorn7 commented May 5, 2015

Can we use both proto versions in parallel in the same binary? We still need the old proto version for the scrape format.

@fabxc

This comment has been minimized.

Copy link
Member Author

fabxc commented May 5, 2015

Yes, that should work.

@juliusv

This comment has been minimized.

Copy link
Member

juliusv commented May 6, 2015

The biggest upside to default values in proto2 was that they were self-documenting - one could simply read in the schema what the defaults were. Now we have to put it into the docstrings and keep it in sync with the code. Meh. It's probably still worth it.

@fabxc

This comment has been minimized.

Copy link
Member Author

fabxc commented May 6, 2015

I'm not convinced that having a config definition file has any real advantages. We have to create manual docs anyway (because of additional explanations and because we cannot expect people to speak protobuf).

The generated code forces us to have an increasing amount of boilerplate code mapping and wrapping into our own types.
As mentioned, the defaults leak into our own code anyway as we have global config stuff. We might just as well have it cleanly in one place.

We don't need external definition for type safety - we get that for free with any parser in Go.

What's left, for me, are comments. There were strong opinions against YAML - with valid reasons I think.
We haven't yet talked about TOML, which is used by various projects and has a solid Go implementation.

Parsing directly into our own types also wipes any issue regarding "JSON for SD" as we can just support both. Also parsing our durations (of which we have many) can actually happen when parsing the file.

@fabxc

This comment has been minimized.

Copy link
Member Author

fabxc commented May 6, 2015

YAML it is 🍰

@fabxc fabxc closed this May 6, 2015

@stapelberg

This comment has been minimized.

Copy link

stapelberg commented May 8, 2015

Why are we breaking the config? Will there be a migration time window and migration path?

@juliusv

This comment has been minimized.

Copy link
Member

juliusv commented May 8, 2015

@stapelberg We had a long discussion about this between quite a few people on IRC about that. I'll send an explanation to the mailing list as to why we're breaking the config file format and why it ended up being YAML.

There is no automated migration tool planned yet (so far only a link to the docs on how to update your config). It can certainly be done, as the new configuration supports a superset of features of the old one. It's just a question of whether it's worth it, and how much time it will save people. I will also mention that in the mail.

@fabxc

This comment has been minimized.

Copy link
Member Author

fabxc commented May 8, 2015

Mostly because of the SD changes described here.

We will document all the steps to upgrade for the next release (i.e., changing the config and rule file comments).
There's also the question whether the upgrade could be automated. We have to put into perspective how much effort manual upgrading is, how much time a well working upgrade tool would cost and delay our progress elsewhere. Finally, we have to consider that we are in version 0.x.

@juliusv

This comment has been minimized.

Copy link
Member

juliusv commented May 8, 2015

@stapelberg We just talked and decided that going from proto2 -> YAML definitely deserves a tool for migrating configs to the new format, so we will provide that. Details TBD.

simonpasquier pushed a commit to simonpasquier/prometheus that referenced this issue Oct 12, 2017

Merge pull request prometheus#670 from prometheus/grobie/update-downl…
…oads-page

Add consul and graphite exporters to downloads page
@lock

This comment has been minimized.

Copy link

lock bot commented Mar 24, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Mar 24, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.