Fix starttime and summary has no quantiles for prometheus receiver #597
Conversation
…as values for these types. It also fixes the issue when summary has no quantiles, allow it to produce a summary with nil snapshots
fivesheep
requested review from
pjanotti,
songy23,
tigrannajaryan and
a team
as code owners
Codecov Report
@@ Coverage Diff @@
## master #597 +/- ##
==========================================
- Coverage 69.13% 68.97% -0.16%
==========================================
Files 92 93 +1
Lines 6110 6125 +15
==========================================
+ Hits 4224 4225 +1
- Misses 1665 1679 +14
Partials 221 221
Continue to review full report at Codecov.
|
…receiver-starttime # Conflicts: # receiver/prometheusreceiver/internal/metricsbuilder.go # receiver/prometheusreceiver/internal/metricsbuilder_test.go # receiver/prometheusreceiver/internal/ocastore_test.go # receiver/prometheusreceiver/internal/transaction.go # receiver/prometheusreceiver/metrics_receiver_test.go
…receiver-starttime
|
@rghetia @dinooliva updated as per #598 |
|
the test failure is not related to the PR: |
| @@ -79,44 +105,205 @@ func TestNew(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| // Test data for EndToEnd test | |||
|
|
|||
| var target1Page1 = ` | |||
There was a problem hiding this comment.
@fivesheep - yes, the endtoend test does help clarify things. A couple of comments:
- Adding a some comments on the target pages about what's being tested would be helpful.
- It looks like resets aren't tested - adding that to a test page would also be worthwhile
- It also looks like empty histogragms/summaries aren't tested here either - I think they would be worthwhile to have too.
There was a problem hiding this comment.
sure will do.
I just checked the reset cases, it seems like when it resets, the adjuster will discard the value (use it as initial) when a reset is detected. Is it an expected behavior? I thought it would use the timestamp from previous scrape as starttime, and value as it is (starting from 0). it's not right or wrong here, just a decision of best-effort. if this is what we want, I can keep the test case this way.
There was a problem hiding this comment.
Yes, a reset acts like an initial point since we don't know when the reset actually occurred. Using the timestamp from the previous scrape does give a much more constrained lower bound than the initial point and may be worth considering - I need to think through the implications.
@rghetia - thoughts?
There was a problem hiding this comment.
It is better to treat reset as initial point. Consider this
0. At time T0 a job reported data
- At time T1 a job stops reporting for whatever reason.
- At time T2 the job resets
- At time T3 the job resumes reporting.
If time lag between T0 and T3 is within scraping interval then it is okay to use T0 as a starttime. Otherwise it will provide incorrect data. I would always prefer no-data vs incorrect data.
…iver-starttime # Conflicts: # receiver/prometheusreceiver/internal/metrics_adjuster.go
|
@songy23 merged |
|
Will merge this PR by EOD tomorrow if there're no further concerns/comments. |
To address start time for counter/histogram/summary, and take delta as values for these types.
For issue: #588
It also fixes the issue when summary has no quantiles, allow it to produce a summary with nil snapshots
For issue: #589
cc @dinooliva @rghetia