-
Notifications
You must be signed in to change notification settings - Fork 9.8k
Add created timestamps to prompb #12936
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
Conversation
cc @bwplotka @macxamin @TheSpiritXIII |
ed6690e
to
5797413
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
5797413
to
965db7a
Compare
|
||
google.protobuf.Timestamp created_timestamp = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be optional, but this deprecated library gogo-protobuf does not support optional fields 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are lots of optional fields in the examples, e.g. https://github.com/gogo/protobuf/blob/f67b8970b736e53dbd7d0a27146c8f1ac52f74e5/test/example/example.proto#L50
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not a gogo vs. official protobuf problem, it's a proto2 vs proto3 problem.
proto3 doesn't distinguish between optional and required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When removing the optional, this is the error I get in CI:
io/prometheus/client/metrics.proto: is a proto3 file that contains optional fields, but code generator protoc-gen-gogofast hasn't been updated to support optional fields in proto3. Please ask the owner of this code generator to support proto3 optional.--gogofast_out:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does
google.protobuf.Timestamp created_timestamp = 3 [(gogoproto.nullable) = true];
Work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat. Would that maybe even help with this issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In different news: Note that essentially everything is (formally) optional
in the proto2 definition. And all of these fields are now "normal" proto3 fields. Same can be done for the created_timestamp. The problem is that then we cannot distinguish between "unset" and "set to default value", but maybe the suggested [(gogoproto.nullable) = true]
helps with that.
About the possibility to set optional
at all: I'm surprised that this is possible now (again). Here's the original rationale for not allowing optional
: protocolbuffers/protobuf#2497 (which is the same rational why all non-repeated fields are marked optional
in proto2).
Apparently, optional
was brought back in protobuf v3.15, with the aim to enable detection if a field was unset, which [(gogoproto.nullable) = true]
should also allow. So I guess we'll go with the latter, and once we move on from gogoproto to another protobuf implementation (that is actually maintained), we can change this to optional
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noice, I wasn't aware that we could work around this way :)
Thanks for the suggestion!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks everybody for helping!
@@ -51,6 +51,8 @@ message Gauge { | |||
message Counter { | |||
double value = 1; | |||
Exemplar exemplar = 2; | |||
|
|||
google.protobuf.Timestamp created_timestamp = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it go on Metric
rather than on the individual sub-components?
And be an int64
like timestamp_ms
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm basically copying what was already merged in client_model. Unfortunately the proto is duplicated in this repo 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is indeed just a port to proto3 from client_model. (And FTR, since created_timestamp only exists for some metric types, it would be a mistake to put in on Metric.)
Signed-off-by: Arthur Silva Sens <arthur.sens@coralogix.com>
965db7a
to
35a06bf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Minor nit, I guess we can fix later - thanks! 💪🏽
@@ -147,4 +153,4 @@ message MetricFamily { | |||
string help = 2; | |||
MetricType type = 3; | |||
repeated Metric metric = 4 [(gogoproto.nullable) = false]; | |||
} | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add empty line back (just convention)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add empty line back (just convention)
I'm surprised that the linter allowed this 🤔
NOTE: As Björn mentioned this is just copy of changes in client_model |
Signed-off-by: Arthur Silva Sens <arthur.sens@coralogix.com>
Signed-off-by: Arthur Silva Sens <arthur.sens@coralogix.com> Signed-off-by: Levi Harrison <git@leviharrison.dev>
Signed-off-by: Arthur Silva Sens <arthur.sens@coralogix.com> Signed-off-by: Levi Harrison <git@leviharrison.dev>
Signed-off-by: Arthur Silva Sens <arthur.sens@coralogix.com> Signed-off-by: Levi Harrison <git@leviharrison.dev>
Signed-off-by: Arthur Silva Sens <arthur.sens@coralogix.com> Signed-off-by: Levi Harrison <git@leviharrison.dev>
Adds created timestamps to prompb.
Adding this in a separate PR because #12733 is getting too big