Parse created timestamps from Prometheus Protobuf#12973
Parse created timestamps from Prometheus Protobuf#12973bwplotka merged 1 commit intoprometheus:mainfrom
Conversation
bwplotka
left a comment
There was a problem hiding this comment.
Epic! One suggestion and LGTM! 💪🏽
model/textparse/openmetricsparse.go
Outdated
| return true | ||
| } | ||
|
|
||
| // CreatedTimestamp returns false because OpenMetricsParser does not support created timestamps. |
There was a problem hiding this comment.
| // CreatedTimestamp returns false because OpenMetricsParser does not support created timestamps. | |
| // CreatedTimestamp returns false because OpenMetricsParser does not support created timestamps (yet). |
There was a problem hiding this comment.
Can you add quick issue about it?
| var foundCT *types.Timestamp | ||
| switch p.mf.GetType() { | ||
| case dto.MetricType_COUNTER: | ||
| foundCT = p.mf.GetMetric()[p.metricPos].GetCounter().GetCreatedTimestamp() | ||
| case dto.MetricType_SUMMARY: | ||
| foundCT = p.mf.GetMetric()[p.metricPos].GetSummary().GetCreatedTimestamp() | ||
| case dto.MetricType_HISTOGRAM, dto.MetricType_GAUGE_HISTOGRAM: | ||
| foundCT = p.mf.GetMetric()[p.metricPos].GetHistogram().GetCreatedTimestamp() | ||
| default: | ||
| } | ||
| if foundCT == nil { | ||
| return false | ||
| } | ||
| *ct = *foundCT | ||
| return true |
There was a problem hiding this comment.
There is a way to write this so we don't have to define foundCT variable - perhaps it would be more readable?
| var foundCT *types.Timestamp | |
| switch p.mf.GetType() { | |
| case dto.MetricType_COUNTER: | |
| foundCT = p.mf.GetMetric()[p.metricPos].GetCounter().GetCreatedTimestamp() | |
| case dto.MetricType_SUMMARY: | |
| foundCT = p.mf.GetMetric()[p.metricPos].GetSummary().GetCreatedTimestamp() | |
| case dto.MetricType_HISTOGRAM, dto.MetricType_GAUGE_HISTOGRAM: | |
| foundCT = p.mf.GetMetric()[p.metricPos].GetHistogram().GetCreatedTimestamp() | |
| default: | |
| } | |
| if foundCT == nil { | |
| return false | |
| } | |
| *ct = *foundCT | |
| return true | |
| switch p.mf.GetType() { | |
| case dto.MetricType_COUNTER: | |
| *ct = p.mf.GetMetric()[p.metricPos].GetCounter().GetCreatedTimestamp() | |
| case dto.MetricType_SUMMARY: | |
| *ct = p.mf.GetMetric()[p.metricPos].GetSummary().GetCreatedTimestamp() | |
| case dto.MetricType_HISTOGRAM, dto.MetricType_GAUGE_HISTOGRAM: | |
| *ct = p.mf.GetMetric()[p.metricPos].GetHistogram().GetCreatedTimestamp() | |
| } | |
| return ct != nil |
There was a problem hiding this comment.
I tried a few combinations without success :/
*ct = p.mf.GetMetric()[p.metricPos].GetCounter().GetCreatedTimestamp() doesn't compile with the error:
cannot use p.mf.GetMetric()[p.metricPos].GetCounter().GetCreatedTimestamp() (value of type *types.Timestamp) as types.Timestamp value in assignment
*ct = *p.mf.GetMetric()[p.metricPos].GetCounter().GetCreatedTimestamp() causes a nilpointer exception if the metric doesn't have a created timestamp set.
So far, with this extra variable was the only way I found to make it work 😕
There was a problem hiding this comment.
Looking at how retrieving exemplars works in the protobuf parser, I don't think there are other alternatives. I can rename the variable foundCT to ctProto if you think that's more clear though!
There was a problem hiding this comment.
some pointer magic needed - it is possible I think, but let's merge simplified version then (not a biggie)
Signed-off-by: Arthur Silva Sens <arthur.sens@coralogix.com>
06422b0 to
598131d
Compare
|
Part of #6541 |
Another PR to make #12733 easier to review
This PR implements parsing created timestamps for the Prometheus Protobuf parser