feat: traces block height and timestamp for InfluxDB #1062
Conversation
staheri14
force-pushed
the
sanaz/log-block-time-influx
branch
from
15fe9af
to
061b926
Compare
There was a problem hiding this comment.
also a note for other reviewers, part of the reasoning for adding this here was to so that we have the option to query from a single source
we could query this data directly using rpc or get aggregated data using the existing metrics (assuming we query prometheus directly), but now we could use influx with the rest of the trace data
pkg/trace/schema/consensus.go
Outdated
| // following schema: | ||
| // | ||
| // | time | height | timestamp | | ||
| BlockTable = "consensus_block" |
There was a problem hiding this comment.
perhaps?
| BlockTable = "consensus_block" | |
| BlockTable = "consensus_block_time" |
There was a problem hiding this comment.
The reason I initially avoided the _time suffix was that we may actually want to trace further attributes for each block. Nevertheless, I incorporated your suggestion!
There was a problem hiding this comment.
we may actually want to trace further attributes for each block.
exactly, this was actually the same reason I think we should change it. If we want further metrics, then we will use a new table, but the name "consensus_block" is already used by the block time
There was a problem hiding this comment.
we may actually want to trace further attributes for each block.
exactly, this was actually the same reason I think we should change it. If we want further metrics, then we will use a new table, but the name "consensus_block" is already used by the block time
So, does this mean that for each additional metric associated with the block, a new table will need to be created? For instance, if we intend to store the DataHash of the block header, would it be necessary to establish another table with a |time|height|data_hash| schema? Not that it is a big issue, but there seems to be potential redundancy, such as the block height being recorded in multiple tables.
There was a problem hiding this comment.
ahhh I see okay that makes sense
There was a problem hiding this comment.
I'll defer to you on if we want to change it back. We can also change it when/if we add more things to this table.
staheri14
changed the title
## Description adds more things to the block table, see #1062 (comment) for further context. part of #1056 --------- Co-authored-by: Sanaz Taheri <35961250+staheri14@users.noreply.github.com>
## Description To accurately gauge block time, it's essential to track the timestamps of committed blocks. The modifications made in this PR incorporate this feature. Incremental work toward #1056
## Description adds more things to the block table, see #1062 (comment) for further context. part of #1056 --------- Co-authored-by: Sanaz Taheri <35961250+staheri14@users.noreply.github.com>
Description
To accurately gauge block time, it's essential to track the timestamps of committed blocks. The modifications made in this PR incorporate this feature.
Incremental work toward #1056