Skip to content

Conversation

ArthurSens
Copy link
Member

Adds created timestamps to prompb.

Adding this in a separate PR because #12733 is getting too big

@ArthurSens
Copy link
Member Author

cc @bwplotka @macxamin @TheSpiritXIII

@ArthurSens ArthurSens force-pushed the create-timestamp-proto branch from ed6690e to 5797413 Compare October 5, 2023 13:06
Copy link

@TheSpiritXIII TheSpiritXIII left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@maxamins maxamins left a comment

Choose a reason for hiding this comment

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

LGTM

@ArthurSens ArthurSens force-pushed the create-timestamp-proto branch from 5797413 to 965db7a Compare October 6, 2023 16:16
Comment on lines 54 to 55

google.protobuf.Timestamp created_timestamp = 3;
Copy link
Member Author

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 🙃

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

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.

Copy link
Member Author

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: 

Copy link
Contributor

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?

Copy link
Member

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?

Copy link
Member

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.

Copy link
Member Author

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!

Copy link
Member

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;
Copy link
Member

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?

Copy link
Member Author

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 😕

Copy link
Member

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>
@ArthurSens ArthurSens force-pushed the create-timestamp-proto branch from 965db7a to 35a06bf Compare October 9, 2023 18:38
Copy link
Member

@bwplotka bwplotka left a 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];
}
}
Copy link
Member

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)

Copy link
Member Author

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 🤔

@bwplotka bwplotka merged commit 4b9c19f into prometheus:main Oct 10, 2023
@bwplotka
Copy link
Member

NOTE: As Björn mentioned this is just copy of changes in client_model

@ArthurSens ArthurSens deleted the create-timestamp-proto branch October 10, 2023 14:15
LeviHarrison pushed a commit to LeviHarrison/prometheus that referenced this pull request Oct 15, 2023
Signed-off-by: Arthur Silva Sens <arthur.sens@coralogix.com>
LeviHarrison pushed a commit to LeviHarrison/prometheus that referenced this pull request Oct 15, 2023
Signed-off-by: Arthur Silva Sens <arthur.sens@coralogix.com>
Signed-off-by: Levi Harrison <git@leviharrison.dev>
LeviHarrison pushed a commit to LeviHarrison/prometheus that referenced this pull request Oct 15, 2023
Signed-off-by: Arthur Silva Sens <arthur.sens@coralogix.com>
Signed-off-by: Levi Harrison <git@leviharrison.dev>
LeviHarrison pushed a commit to LeviHarrison/prometheus that referenced this pull request Oct 15, 2023
Signed-off-by: Arthur Silva Sens <arthur.sens@coralogix.com>
Signed-off-by: Levi Harrison <git@leviharrison.dev>
LeviHarrison pushed a commit to LeviHarrison/prometheus that referenced this pull request Oct 15, 2023
Signed-off-by: Arthur Silva Sens <arthur.sens@coralogix.com>
Signed-off-by: Levi Harrison <git@leviharrison.dev>
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.

7 participants