celestiaorg / go-header

Go library with all the services needed to request, sync and store blockchain headers.
Apache License 2.0
17 stars 16 forks source link

feat(syncer/metrics): add metrics to syncer #136

Closed vgonkivs closed 5 months ago

vgonkivs commented 6 months ago

Overview

Add metrics to syncer

Checklist

codecov-commenter commented 6 months ago

Codecov Report

Attention: 75 lines in your changes are missing coverage. Please review.

Comparison is base (28ff21c) 63.60% compared to head (0070afe) 63.03%. Report is 13 commits behind head on main.

Files Patch % Lines
sync/metrics.go 11.90% 69 Missing and 5 partials :warning:
sync/sync_head.go 83.33% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #136 +/- ## ========================================== - Coverage 63.60% 63.03% -0.58% ========================================== Files 39 39 Lines 3366 3476 +110 ========================================== + Hits 2141 2191 +50 - Misses 1060 1113 +53 - Partials 165 172 +7 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

vgonkivs commented 6 months ago

Снимок экрана 2023-12-15 в 12 47 45 Снимок экрана 2023-12-15 в 12 46 04

Please note, that peer info is not shown on the second screenshot because only one node was running

Wondertan commented 5 months ago

@vgonkivs, this is a feature, not misc

Wondertan commented 5 months ago

Tracking requestHeaders time feels like a job for tracing rather than metrics. Mainly because traces would provide the same data as metrics with additional insights on sub operations/spans. Although, this two can co-exist with metrics for monitoring anomalies and traces specifically for debugging.

Wondertan commented 5 months ago

For completely synced metric. I think we should add it, as syncLoopStop or similar.

walldiss commented 5 months ago

Tracing is easier for debugging, while metrics are handy to have general overview of app. Having request time histogram will help us to quickly spot that there are some issues and then proceed to traces to investigate with more details.

Completed sync it could be a isSyncedGauge, that does simple async if check on internal syncer state. For request time it would be great to have syncRequestTimeHist, which could track requests finished with their execution time.

Wondertan commented 5 months ago

From our quick sync with @walldiss, we agreed to add a few more metrics:

vgonkivs commented 5 months ago

syncLoopGauge -> why can't we have a simple atomic with values 1/0? It will be enough I believe.

vgonkivs commented 5 months ago

please advice on bucketing(might need opentelemetry bump) -> I have tried buckets in the node repo but it hasn't worked for me.