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

Remove mentions of the Protobuf format #1083

Merged
merged 1 commit into from
Jun 29, 2018
Merged

Remove mentions of the Protobuf format #1083

merged 1 commit into from
Jun 29, 2018

Conversation

lucperkins
Copy link
Contributor

In response to issue #927. Although there are still some stray mentions of the Protobuf format in the documentation, this PR should remove the mentions that are the most likely to cause confusion and consternation. In the future we should conduct a more thorough sweep.

that implement this format for you. If your preferred language doesn't have a client
library you can [create your own](/docs/instrumenting/writing_clientlibs/).

NOTE: Some earlier versions of Prometheus supported an exposition format based on
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The NOTE: here should probably still be NOTE: **NOTE:**, as the initial NOTE: is translated into a formatting directive that creates a call-out box and is otherwise not displayed, see https://prometheus.io/docs/instrumenting/exposition_formats/

Or was your intention to get rid of the displayed "NOTE" text within that box?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm aware of the NOTE syntax. In my view the styling makes it unnecessary to include NOTE but I'm not dogmatic about that. Your call.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I just tried removing it on the live site and it looks a bit odd to me in context without the "NOTE" text:

note

I'd be for keeping it. If we do decide to remove it, we should make that a conscious decision and remove the extra callout words throughout the site, not just in one instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'll make that change. In a future PR I'll adjust the template of admonition blocks so that the admonition type is included automatically.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea!


### Text format details

The protocol is line-oriented. A line-feed character (`\n`) separates lines.
The last line must end with a line-feed character. Empty lines are ignored.
Prometheus text-based format is line oriented. Lines are separated by a line
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should "Prometheus" be "Prometheus's" (or "Prometheus'") here?


Within a line, tokens can be separated by any number of blanks and/or tabs (and
have to be separated by at least one if they would otherwise merge with the
previous token). Leading and trailing whitespace is ignored.
must separated by at least one if they would otherwise merge with the previous
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

must -> must be

metric_name [
"{" label_name "=" `"` label_value `"` { "," label_name "=" `"` label_value `"` } [ "," ] "}"
] value [ timestamp ]
`metric_name` and `label_name` have the usual Prometheus expression language restrictions:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following bullet points that are introduced here don't actually further describe this sentence, they are completely separate points. So either everything should be a bullet point (with a different intro statement) or nothing should be...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, okay. I'm just going to restructure this list.

`metric_name` and `label_name` have the usual Prometheus expression language restrictions. `label_value` can be any sequence of UTF-8 characters, but the backslash, the double-quote, and the line-feed characters have to be escaped as `\\`, `\"`, and `\n`, respectively.
`value` is a float, and timestamp an `int64` (milliseconds since epoch, i.e. 1970-01-01 00:00:00 UTC, excluding leap seconds), represented as required by the [Go strconv package](http://golang.org/pkg/strconv/) (see functions `ParseInt` and `ParseFloat`). In particular, `Nan`, `+Inf`, and `-Inf` are valid values.
* `label_value` can be any sequence of UTF-8 characters, but the backslash, the double-quote, and the line-feed characters have to be escaped as `\\`, `\"`, and `\n`, respectively.
* `value` is a float
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Period missing on this bullet point (the others have periods).

`value` is a float, and timestamp an `int64` (milliseconds since epoch, i.e. 1970-01-01 00:00:00 UTC, excluding leap seconds), represented as required by the [Go strconv package](http://golang.org/pkg/strconv/) (see functions `ParseInt` and `ParseFloat`). In particular, `Nan`, `+Inf`, and `-Inf` are valid values.
* `label_value` can be any sequence of UTF-8 characters, but the backslash, the double-quote, and the line-feed characters have to be escaped as `\\`, `\"`, and `\n`, respectively.
* `value` is a float
* the `timestamp` is an `int64` (milliseconds since epoch, i.e. 1970-01-01 00:00:00 UTC, excluding leap seconds), represented as required by the [Go strconv package](http://golang.org/pkg/strconv/) (see functions [`ParseInt`](https://golang.org/pkg/strconv/#ParseInt) and [`ParseFloat`](https://golang.org/pkg/strconv/#ParseFloat)). In addition to standard integers, `Nan`, `+Inf`, and `-Inf` are valid values representing not a number, positive infinity, and negative infinity, respectively.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last sentence and the floaty bit of the previous one are actually still about the sample value, which would be the previous bullet point.

the ingestion behavior is undefined.

#### Histograms and summaries
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When introducing these new sub-headers, we'll then have to introduce them for all sub-sections of the text format, including the example section below (the example is no longer about histograms and summaries in particular, but about the text format in general).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, you're right. Missed this one.

@juliusv
Copy link
Member

juliusv commented Jun 28, 2018

👍 besides comments, thanks!

@lucperkins
Copy link
Contributor Author

@juliusv Okay, I think I've addressed all of the issues you've pointed out. One question, though: in the "Optional Text Representation" section you'll see that Protobuf is still mentioned as an optional format. Is that still the case? Or should mention be removed entirely?

* `metric_name` and `label_name` carry the usual Prometheus expression language restrictions.
* `label_value` can be any sequence of UTF-8 characters, but the backslash (`\`, double-quote (`"`}, and line feed (`\n`) characters have to be escaped as `\\`, `\"`, and `\n`, respectively.
* `value` is a float.
* the `timestamp` is an `int64` (milliseconds since epoch, i.e. 1970-01-01 00:00:00 UTC, excluding leap seconds), represented as required by the [Go strconv package](http://golang.org/pkg/strconv/) (see functions [`ParseInt`](https://golang.org/pkg/strconv/#ParseInt) and [`ParseFloat`](https://golang.org/pkg/strconv/#ParseFloat)). In addition to standard integers, `Nan`, `+Inf`, and `-Inf` are valid values representing not a number, positive infinity, and negative infinity, respectively.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment still applies: d6e75b7#r198996763

The last sentence is about sample values and the ParseFloat bit of the previous sentence too (since sample values are floats, timestamps are ints).

@juliusv
Copy link
Member

juliusv commented Jun 28, 2018

One question, though: in the "Optional Text Representation" section you'll see that Protobuf is still mentioned as an optional format. Is that still the case? Or should mention be removed entirely?

I am not aware of anyone ever having implemented any of those optional text representations. I think we should just nuke that entire section, as it has little bearing on reality.

@lucperkins
Copy link
Contributor Author

@juliusv Makes sense. Less material --> less to maintain 👍

Signed-off-by: lucperkins <lucperkins@gmail.com>
@lucperkins lucperkins merged commit deda44c into prometheus:master Jun 29, 2018
@lucperkins lucperkins deleted the lperkins/issue-927-protobuf branch June 29, 2018 16:33
@juliusv
Copy link
Member

juliusv commented Jun 29, 2018

@lucperkins #1083 (comment) still needs to be fixed

@lucperkins lucperkins restored the lperkins/issue-927-protobuf branch June 30, 2018 00:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants