Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move stats to metrics #226

Closed
wants to merge 2 commits into from
Closed

Move stats to metrics #226

wants to merge 2 commits into from

Conversation

bogdandrutu
Copy link
Member

The main idea is to reduce confusion about why two top level APIs exists to record metrics/stats/measurement.

We still need to offer the ability to record raw measurements as well as pre-defined metrics (including aggregation and labels).

The structure of the Metrics package is not final:

  1. Do we need MetricsRegistry or we move all the methods to Meter? The SDK will allow users to set Filters that apply to all created metrics (similar with metric registry in Micrometer for example).
  2. The Views API in the SDK is not yet moved to metrics.

Copy link
Member

@songy23 songy23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed the high-level idea that merges stats into metrics makes the API easier to understand. Though I think in this case we should move View APIs to this package too, since in order to get aggregated metrics from measurements, users still need to define Views. IMO It's a bit weird to put View at a different package.

@bogdandrutu
Copy link
Member Author

View is an implementation detail so they will be in sdk/metrics/*. By implementation detail I mean there can be implementation of this API that don't allow users to define the aggregation - always construct a histogram for example.

@bogdandrutu
Copy link
Member Author

My comment in the description was more to document what else needs to happen.

@yurishkuro
Copy link
Member

Some questions/comments:

  • in many other metrics APIs that allow recording gauges, timers, and/or histograms, the recording API is still about recording the raw measure, but behind the scene the library may or may not perform some form of aggregation, like LastValue for gauges, or Histogram for timers. So the distinction between raw measure and Gauge/Histogram is not clear to me, since from instrumentation point of view it's always recording the raw value.
  • on the other hand, counters seem to be fundamentally different and can never be captured via raw measure, is that correct? This breaks the mental model for me.
  • being able to change aggregation without touching instrumentation is great - I often wanted to see something like queue_length not as a gauge but as a histogram (incidentally tally didn't support histograms for non-timer values)
  • application of request-scoped tags is also very tricky. As @bogdandrutu said, there are some stats that don't make sense to be labeled with request-scoped tags, e.g. CPU utilization. For measures where we do want to apply context tags, the ability to define views also provides a place to define context tag rules, potentially on a per-measure basis, e.g. "allow tags A and B for measureX, but only tag B for measure Y" (however, see next item).
  • the requirement to define views seems very burdensome, especially because they need to reference the actual measure objects. In a complex, multi-layer system like Jaeger backend, the metrics defined by the individual components are usually encapsulated within those components, and it's not feasible to have a single place in the app where I can define views for all measures from all components. On the other hand, if views are defined internally by the components themselves, then it mostly makes the views redundant, since the components can define aggregations at the time of constructing the measures, which is what typically happens with most other metrics APIs where you explicitly ask for Counter, Gauge, Histogram. I'd be interested to see how more complex systems using OC deal with this encapsulation issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants