Expose metrics views in a slice

[anacrolix]

Exposes all the views in a slice for convenience, and because they're self-describing.

[frrist]

Looks good! Left a non-blocking question.

[frrist]

This change means consumers of this package will no longer be able to register just some of the views, is that correct?

I liked the idea of having the ability to pick and choose which views my code registered, for example maybe I am only interested in ReceivedBytesView and SentBytesView. We could preserve this functionally by leaving the views assigned to exported variables while also adding them to an exported slice, as was done here. What do you think?

[anacrolix]

That seems reasonable, but I think it binds the API to particular views that may not have relevance going forward. I'm not sure there's ever a circumstance where all the views would not be exported (now I think perhaps DefaultViews would be a better name). They can still be found in the slice, and are self-describing. Further, should users want to roll their own views, or modify them, they can do that, the measures are exported for doing so. I suspect in future, that with all the libp2p and other packages having various metrics and views exposed, we'll need to switch to some automatic registration anyway to avoid fatigue traversing all the packages of interest and selecting individual views.

If you feel strongly about it, I'll leave them exported for now, and provide the slice just for convenience.

[frrist]

No strong feelings, you make a good point about binding the API to views that may become irrelevant -- this lgtm as is.

[raulk]

This is ok for now, but I expect we'll adjust this API going forward.

Exporting views in a static var is the simple path now. But we are introducing the notion of a service-oriented Host.

If we put these views behind an interface (e.g. Measurable), the DI entrypoint could inspect all services and interrogate them for default views.

With the var-based construction, it's not possible to do so unless we use reflection.

[bigs]

lgtm

[lanzafame]

@anacrolix I made a valid case against this when I put #317 up. So I would have appreciated a ping on this PR.

[lanzafame]

All you needed to do was put the views in a slice, i.e.

var DefaultViews = []*view.View{
    ReceivedMessagesView,
    ReceivedMessageErrorsView,
   ...
}

@anacrolix @raulk this is a breaking change from 0.0.9 so either it shouldn't be under 0.0.10 or should be reverted for a solution that is backwards compatible.

[raulk]

@lanzafame – sorry I missed your point in favour of exporting individual views in the cited comment :-(

All expressed points of view are reasonable. I wonder if we can engineer a best practice across IPFS [cluster], Filecoin and libp2p that:

1. strikes the right balance between offering recommended views and composing them from scratch
2. does so in a way that is flexible for the owning component to change which stats are collected, and how, under the hood.

Looking again at this PR, the rationale seems flawed. Omitting views from public exports, in order to enable the owning component to change those views, all the while we keep measurements exported, is incoherent. After all, any big enough change in stats collection will probably end up altering the measurements themselves (which are publicly exposed). So API breakage is inevitable.

Moreover, this particular approach is buggy because it leaks write access to internal state via the slice.

In general, I think API users should not refer to views and measurements statically. An alternative that springs to mind is a "metrics repository" abstraction, onto which components load their measurements and views. Getting inexistent measurements and views from this component would return an error, but would not introduce API breakage.