OTLP receiver: Convert start timestamps to Prometheus zero samples #14759
Conversation
dc22c79 to
5c6da56
Compare
| return | ||
| } | ||
| if value != 0 && startTs > 0 && startTs == ts { | ||
| // NB: We currently handle only start timestamps which are the same as the data point timestamp. |
There was a problem hiding this comment.
I don't like this phrasing; I would say we handle all start timestamps, as long as we see the first point.
Suggest linking to the umbrella issue for people wanting more explanation, and leave it out of the code.
There was a problem hiding this comment.
@bboreham are you suggesting to make the comment only link to the umbrella issue instead? Uncertain if you mean to put the umbrella issue in the comment or in the PR description.
There was a problem hiding this comment.
I put the issue link in the comment, please let me know if you had something else in mind.
| ts := c.addSample(infBucket, infLabels) | ||
|
|
||
| bucketBounds = append(bucketBounds, bucketBoundsData{ts: ts, bound: math.Inf(1)}) | ||
| c.addExemplars(pt, bucketBounds) | ||
|
|
||
| startTimestamp := pt.StartTimestamp() | ||
| if settings.ExportCreatedMetric && startTimestamp != 0 { | ||
| otelStart := pt.StartTimestamp() |
There was a problem hiding this comment.
Not in love with this name; everything around here is Otel.
AFAICS the difference is that this one is in nanoseconds while startTimestamp is in milliseconds; perhaps that could be used in the name?
There was a problem hiding this comment.
I renamed startTimestamp to startTimestampMs and otelStart to startTimestampNs. PTAL.
5c6da56 to
cbbaf41
Compare
| @@ -540,6 +549,18 @@ func (c *PrometheusConverter) addTimeSeriesIfNeeded(lbls []prompb.Label, startTi | |||
| } | |||
| } | |||
|
|
|||
| // handleStartTime adds a zero sample 1 millisecond before ts iff startTs == ts. | |||
There was a problem hiding this comment.
It might be worth mentioning that we're doing this because PRWv1 doesn't support Created Timestamps.
Once we migrate the translation layer to the PRWv2 Protocol, this won't be needed and we'll just let the PRWv2 handler take care of StartTimeUnixNano
There was a problem hiding this comment.
Added to the doc comment, PTAL.
There was a problem hiding this comment.
Just pushed the commit altering the doc comment, had somehow omitted to push it earlier 🤦
There was a problem hiding this comment.
@bwplotka do you mind giving a review here as well? It would be nice to get some eyes from someone outside Grafana
c6656a6 to
79b16d9
Compare
|
If reviewers prefer, I can factor out the added tests into another PR (dependency of this one). |
| // The reason for doing this is that PRW v1 doesn't support Created Timestamps. After switching to PRW v2's direct CT support, | ||
| // make use of its direct support fort Created Timestamps instead. |
There was a problem hiding this comment.
| // The reason for doing this is that PRW v1 doesn't support Created Timestamps. After switching to PRW v2's direct CT support, | |
| // make use of its direct support fort Created Timestamps instead. | |
| // We append `StartTimeUnixNano` as an extra zero sample because PRWv1 doesn't have support for Created Timestamps. Once the OTLP translation layer migrates to PRWv2, we can let the PRWv2 handler decide what to do with its Created Timestamp and this method won't be necessary. |
There was a problem hiding this comment.
this method won't be necessary.
Isn't it possible that this method will still be necessary for converting to PRW v2's created timestamp representation? I can't say for sure what the PRW v2 conversion code will look like.
e1030bd to
5f1c411
Compare
|
I rebased the PR with main changes that included some of the tests files also present here resulting in a much smaller PR for review 👍 |
5f1c411 to
6fd88cc
Compare
|
Rebased the PR on main, and marked ready for review. |
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Co-authored-by: Arthur Silva Sens <arthursens2005@gmail.com> Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
6fd88cc to
aabced2
Compare
|
Hi, I believe things moved on a bit in the fork we are using to test this out. |
|
Hello from the bug-scrub! @aknuds1 do you think you will get back to this? I believe @krajorama was also looking at Created Timestamps. |
|
@krajorama Do you want to take this PR over? |
|
FYI: We do want to long term deprecate zero injection feature... ideally. And use proper CT storage for CT handling 🙈 just saying, this is not perfect solution - depending on rationales to implement it here it might make sense. |
Modify the OTLP receiver to, if the
--created-timestamp-zero-ingestionfeature flag is true, convert data point start timestamps to Prometheus zero samples 1 millisecond before the sample itself, iff the following is true: The OTel start timestamp is greater than zero and equal to the data point timestamp.I'm also adding tests, copied from https://github.com/open-telemetry/opentelemetry-collector-contrib, with attribution.
Fixes #14600.