chore(proposervm): timestamp metrics for block acceptance #3680
Conversation
Co-authored-by: Darioush Jalali <darioush.jalali@avalabs.org> Signed-off-by: Arran Schlosberg <519948+ARR4N@users.noreply.github.com>
| defer func() { | ||
| require.NoError(t, proVM.Shutdown(ctx)) | ||
| }() |
There was a problem hiding this comment.
| defer func() { | |
| require.NoError(t, proVM.Shutdown(ctx)) | |
| }() | |
| t.Cleanup(func() { | |
| require.NoError(t, proVM.Shutdown(ctx)) | |
| }) |
There was a problem hiding this comment.
I'll defer to @StephenButtolph on this due to consistency within the file.
Is there any real difference between the two? If anything I think it would be better to call t.Cleanup() inside initTestProposerVM() and remove it from everywhere else, but not in this PR.
There was a problem hiding this comment.
We switched to using t.Cleanup in some of our other packages (in holistic passes). Seems like something that could be pulled out into a followup
| {innerBlockTypeMetricLabel, innerTime}, | ||
| {outerBlockTypeMetricLabel, outerTime}, | ||
| } | ||
| for _, tt := range tests { |
There was a problem hiding this comment.
nit why tt and not test 👀 I suppose not to conflict with t? 🤔 But test is nice and short too 😅 Just a general comment for future code, please no need to change it here.
There was a problem hiding this comment.
Yeah tt to not shadow t. It was common enough in the Google code base (along with tc for test case) that it was generally known exactly what it was (like ctx) so I picked it up as a habit, but appreciate that that may not be the case outside. I tend to keep using it over test because of the number of times it gets used.
There was a problem hiding this comment.
I'd probably slightly prefer test as well - but I don't really care tbh
Co-authored-by: Quentin McGaw <quentin.mcgaw@avalabs.org> Signed-off-by: Arran Schlosberg <519948+ARR4N@users.noreply.github.com>
There was a problem hiding this comment.
# HELP avalanche_proposervm_last_accepted_timestamp timestamp of the last block accepted
# TYPE avalanche_proposervm_last_accepted_timestamp gauge
avalanche_proposervm_last_accepted_timestamp{block_type="inner",chain="C"} 1.738141385e+09
avalanche_proposervm_last_accepted_timestamp{block_type="inner",chain="P"} 1.738164828e+09
avalanche_proposervm_last_accepted_timestamp{block_type="inner",chain="X"} 1.738153495e+09
avalanche_proposervm_last_accepted_timestamp{block_type="proposervm",chain="C"} 1.738141385e+09
avalanche_proposervm_last_accepted_timestamp{block_type="proposervm",chain="P"} 1.738164828e+09
avalanche_proposervm_last_accepted_timestamp{block_type="proposervm",chain="X"} 1.738153495e+09
Why this should be merged
Allows for alerting should
proposervmblock times drift too far from their inner blocks.How this works
Adds a new
proposervmvector metric for last-accepted timestamp.Lightly held opinion: although this introduces a redundant metric (
proposervm_last_accepted_timestampwithblock_type=proposervmis identical tosnowman_last_accepted_timestamp), I don't think we should remove the latter because it has a natural partner insnowman_last_accepted_height. Also, while usage ofproposervmis currently required, if this were to change then there would be a need for thesnowmanmetric but it would likely be forgotten. In summary, I think a little redundancy is ok in return for snowman-metric completeness.How this was tested
Unit test.
Need to be documented in RELEASES.md?
Nope.