[ADR: 013] Observability

[tac0turtle]

• Inital ADR for metrics with in the SDK
• extending metrics to be module specific as well

ref #4782

Signed-off-by: Marko Baricevic marbar3778@yahoo.com

• Targeted PR against correct branch (see CONTRIBUTING.md)

• Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

• Wrote tests

• Updated relevant documentation (docs/)

• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md

• Re-reviewed Files changed in the github PR explorer

---

For Admin Use:

• Added appropriate labels to PR (ex. wip, ready-for-review, docs)
• Reviewers Assigned
• Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[tac0turtle]

[alexanderbez]

formatting is off

[alexanderbez]

Can we rename this ADR to Telemetry or Observability?

[jackzampolin]

🚢

[fedekunze]

is this R4R @marbar3778 ?

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[tnachen]

itself.To -> itself. To

[tnachen]

Gauge, I think we do need description and also what units are we measuring io as well.

[fedekunze]

@marbar3778 bump

[tnachen]

@marbar3778 updated based on our discussion, let me know if this looks good or not.
@alexanderbez @fedekunze PTAL

[fedekunze]

See comments. CreateMetrics should be general instead of specific to one metric.

[fedekunze]

the CreateMetrics should be generic so that it can be called by the module manager. In this case, you are creating a function only for the specific case of the Size field. Say you want to initialize also the Counter for Transactions, with the following function:

NewCounterFrom(opts prometheus.CounterOpts, labelNames []string) *Counter

How do you know whichlabelsAndValues correspond to which metric? This also applies for Histogram and Summary

[tnachen]

I believe this comes from what Tendermint did and we could change this for sure, but in TM the same labels are being added to every metric that the module wants to expose.

So the workflow is that module owner will just add Metrics they want to track in the metric struct, and to add more metrics you simple add more fields in that struct, and not only tied to one metric per say.

So if we want the flexibility that every metric can have some custom labels that gets added when pulled from Prometheus then we need to perhaps create a different API for module to define these metrics, since you need the metric type, name and the labels associated with it, potentially even global labels that gets applied to every metric like what we have here.

@fedekunze do you know if we need to have this flexibility to definte custom labels on each metrics in the same module? Or having it apply to every metric in that module is good enough?

[fedekunze]

I think it's better to define the labelNames explicitly as params

[tnachen]

you mean CreateMetrics(namespace string, labelNames...string, labelValues ...string)?
It is much easier to iterate if it's one array, alternatively we can do map[string]string too which probably is more safe.

[tnachen]

Actually it's the prometheus API that takes labelsAndValues list as defined above, so if we change it to two lists or even a map, then every module writer needs to convert it themselves. We could provide a helper method but I feel like it's a bit overkill.
@fedekunze are you fine keeping this? you can look at TM example here https://github.com/tendermint/tendermint/blob/c9ef824ddf623a873ca226b98ddb3539c1de1ebc/p2p/metrics.go#L44

[tnachen]

This only added one markdown file so I don't think codecov is accurate here.

[tac0turtle]

dayuummmm