Skip to content

Instantly share code, notes, and snippets.

@MadVikingGod
Last active December 3, 2021 21:34
Show Gist options
  • Save MadVikingGod/e789404f40fefc537c6034fdedcaf69c to your computer and use it in GitHub Desktop.
Save MadVikingGod/e789404f40fefc537c6034fdedcaf69c to your computer and use it in GitHub Desktop.

Comments on an implementor trying to parse the metrics spec.

Views

Type

The current way the spec is stated is there can only be 1 View type, even though it does multiple things. By my count right now it does:

  1. Filter meters.
  2. Remove dimensions.
  3. Rename dimensions.
  4. Add dimensions.
  5. Reaggregate.

From a go perspective this would be accomplished by using an interface, and a set of composable types. As written we would have A large anti-pattern class that is able to do all of this at once.

Recommendation: Change this to specify the minimum features we want from views, what each feature would need, and the general shape of the interface between the meterprovider (or whatever tracks meters) and the meter reader (what ever queries views) eg:

Meter processing flow

I would recommend that view's be a non-optional, and allow for the meter_provider.add_metric_reader method to a shortcut for meter_provider.add_view(select_all_view{}).add_metric_reader. This would simplify metric reader, it would have a consistant interface it would need to read from (the one provided by views) and reduce it's work to iterating through the views it's registered to. It would also have the side effect of having a default behavior when an instrument is not captured by any view, the meter provider would drop all readings. Logistically there could be multiple views on a meter provider, and multiple views registered to a meter reader.

MeterReader

Following the previous recommendation I would change the support of this to say The SDK MUST support multiple views registered to a MetricReader.

I would also change the wording of the MetricReader.Collect invocation on one MetricReader instance SHOULD NOT introduce side-effects to other MetricReader instances.. I think the intent is that actions of one MetricReader should not change the internal state of another.

Contexts

The spec says that views should be able to add dimensions from Context. This implies a few things:

  1. All views have to manage context,
  2. All meters have to provide a context to the views,
  3. The context are only safe to discard when it is reaggeragted or after ALL meter readers read that context. To put this another way is we have to capture that context until the reading with that context goes out of scope.

Recommendation: Push this responsibility onto the instrumentation libraries, and maybe individual syncronous meters. The context shouldn't propogate past the reading of the meter, it is then where the values from the context are put into dimensions.

Control flow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment