Add Prometheus metrics for slot_no #537
Conversation
|
@ksaric, I'm not the owner of |
| -- The metrics we use. | ||
| -- PLEASE do not put this into ENV, it's defeating the purpose | ||
| -- of these constructs. | ||
| data MetricsLayer = MetricsLayer |
There was a problem hiding this comment.
Like a DataLayer, we used names Layer to describe such interfaces. I think the wallet does a similar thing, it's not a first. I think the name fits quite well. It's a layer that can be cascaded (like the caching layer on top of a db layer), and it's an interface, a small surface that "touches" the world.
There was a problem hiding this comment.
The name MetricsSetters is a much better name. It removes the confusing "layer" from the name and "setters" describes exactly what the components of this struct do.
Yes, naming things in hard. The old name had been bugging me since last week and the new name just came to me now.
| emptyMetricsLayer :: MetricsLayer | ||
| emptyMetricsLayer = | ||
| MetricsLayer | ||
| { mlSetNodeBlockHeight = \_ -> pure () |
There was a problem hiding this comment.
This is much clearer to me than const.
I know you were creating some tool, not sure where is it actually, so 🤷♂️ |
ksaric
force-pushed
the
ksaric/CAD-2547
branch
3 times, most recently
from
521694c to
ed0e0ba
Compare
|
bors try+ |
tryBuild succeeded: |
|
|
||
|
|
||
| -- |The metrics we use for Prometheus. | ||
| data Metrics = Metrics |
There was a problem hiding this comment.
Why is it a layer? To me "layer" implies that if we have say 3 layers, A, B and C, that A calls into B and then B calls into C. That is not the case here. Its not a layer, its a Metrics component.
There was a problem hiding this comment.
The term Layer has been used throughout the codebase in all these instances. Please, let's use the name Layer that is a standard term for these things. A Setter is not.
Examples:
- https://github.com/input-output-hk/cardano-wallet/blob/master/lib/core/src/Cardano/Pool/DB.hs#L15
- https://github.com/input-output-hk/cardano-wallet/blob/master/lib/core/src/Cardano/Wallet/Network.hs#L15
- https://github.com/input-output-hk/cardano-wallet/blob/master/lib/core/src/Cardano/Wallet/Transaction.hs#L21
| @@ -66,3 +67,16 @@ newtype BlockId = BlockId Int | |||
| -- @Word64@ is valid as well. | |||
| newtype MetaId = MetaId Int | |||
| deriving (Eq, Show) | |||
There was a problem hiding this comment.
What are MetaId and BlockId . Are they used anywhere else in this repo? I am pretty sure they are not because the names clash with identifiers that are generated by Persisitent in cardano-db/src/Cardano/Db/Schema.hs.
There was a problem hiding this comment.
Same applies to Block and Meta in this same file.
There was a problem hiding this comment.
If I remove BlockId, Meta and Block, everything still compiles. So why are they here?
There was a problem hiding this comment.
They were supposed to be here and used by some other functions in the CardanoDataLayer. For now, they are not used anywhere so if you want, you can drop them.
erikd
force-pushed
the
ksaric/CAD-2547
branch
3 times, most recently
from
58c5fc9 to
0c2888b
Compare
* Allows metrics to be used from cardano-sync package. * Add a metric for slot_no. https://jira.iohk.io/browse/CAD-2547 Co-authored-by: Erik de Castro Lopo <erikd@mega-nerd.com>
erikd
changed the title
There is no point in having Prometheus metrics in |
https://jira.iohk.io/browse/CAD-2547
Also removed the direct dependency, Prometheus is optional and we now have the option to simply ignore Prometheus (for example, tests).
@DavidEichmann If you are the owner of
cardano-db-tool, it currently doesn't start Prometheus.Also, changed the names of the metrics, so I guess I have to inform DevOps after this is merged.