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

API: Are "views" actually required for metrics? #268

Closed
yurishkuro opened this issue May 10, 2019 · 13 comments
Closed

API: Are "views" actually required for metrics? #268

yurishkuro opened this issue May 10, 2019 · 13 comments
Assignees
Labels
API API related issues

Comments

@yurishkuro
Copy link
Member

Extracting from #226 (comment):

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.

@jotak
Copy link

jotak commented May 13, 2019

Hi,
I'm not a true user of open-telemetry / open-census so please note some of my assumptions may not always be correct/accurate about the telemetry API (tell me if I'm misunderstanding something...) However I've been looking quickly into the API and try to evaluate / compare it with micrometer in the context of the vertx-micrometer-metrics module ( https://github.com/vert-x3/vertx-micrometer-metrics/ )

I thought Views were quite nice to avoid having to re-implement something very similar in the consumer code. For instance it is quite common, I think, to bind a measure (ex: Counter) with a set of label keys, and keep a reference to that bound object instead of a reference of the underlying measure. If I understand correctly this is something that the views provide. Example in vertx metrics where I do something similar: https://github.com/vert-x3/vertx-micrometer-metrics/blob/master/src/main/java/io/vertx/micrometer/impl/meters/Counters.java . Of course this is quite a trivial piece of code, but still I wish it was provided out of the box by the metrics API (here in micrometer), and hence better optimized than what I do here.

@yurishkuro
Copy link
Member Author

For instance it is quite common, I think, to bind a measure (ex: Counter) with a set of label keys, and keep a reference to that bound object instead of a reference of the underlying measure.

@jotak could you provide more concrete example of the underlying measure and labels?

@bogdandrutu bogdandrutu changed the title Are "views" actually required for metrics? API: Are "views" actually required for metrics? May 13, 2019
@bogdandrutu
Copy link
Member

@yurishkuro the most useful example for applications that we build are RPC/HTTP stats, and stats about a shared resource like disk spindles.

For RPC/HTTP stats there are couple of benefits/use-cases:

  1. With views user can define their own histogram buckets.
  2. Add extra labels:
    • One very good example is, in the frontend (expect a microservice infrastructure) add tags about this requests (e.g. client_id, or a/b test, etc.).
    • We define a notion of "caller" tag, that can be populated by every service when they make RPC/HTTP requests (this can also be resolved in the RPC/HTTP layer).
    • Another use-case which I didn't see that much requested in oss is to "back-propagate" tags (only from the application back to the RPC layer), but we use it a lot. For example if you do an A/B testing instead of recording again your metrics for all the requests, you annotate all the already available RPC server metrics with these labels.
    • Another super important use-case is that usually the RPC/HTTP stats are not in control of every team they are defined by gRPC for example. So having the ability to change all of these without changing that code is nice.
    • We also allow add/remove views at runtime.

So Views are not "required" they are useful for certain cases as I tried to explain, that is also the main reason we have to support both cases:

  • raw measurements without aggregation and labels define. // These allows users to have more flexibility
  • fully defined metrics with aggregation and labels predefined. // These are faster but not that flexible

We also so the problem with defining views all the time, that's why:

  1. We did create helper methods "InstallMinimumRequiredViewsForRPC".
  2. We plan to allow users to define the views in an yaml file.

@yurishkuro yurishkuro added the API API related issues label May 14, 2019
@tedsuo
Copy link
Contributor

tedsuo commented May 29, 2019

Action Item:

  • @bogdandrutu will write a proposal for this, and we'll have a meeting including @jmacd and other interested parties.
  • Respond to this thread if you are interested in discussing.

@bogdandrutu
Copy link
Member

I am not ready with this because the jetlag hit me too bad.

@SergeyKanzhelev SergeyKanzhelev added this to the API complete milestone May 30, 2019
@yurishkuro
Copy link
Member Author

@bogdandrutu I feel that "raw vs. fully defined" metrics is a false dichotomy, as I explained in #269, because the basic counter doesn't fit in either category.

Views double as both an enrichment layer (extra labels) and an aggregation layer (computing histogram from raw observations), which is fine as long as they are optional. There needs to be an encapsulation principle - if I instantiate a component and pass it a metrics factory, it should be able to define metrics that are exportable without any further handholding. In Jaeger backend we have many layers of components, all defining their own metrics, it's simply not feasible to have them all expose InstallMinimumRequiredViewsForXYZ and have something being responsible for calling those functions before metrics can be emitted.

raw measurements without aggregation and labels defined

Why does "raw" measurement need to be coupled with not having labels?

@bogdandrutu
Copy link
Member

@yurishkuro not sure I understand what you mean by "basic counter", do you refer to a pre-computed counter like for example cpu-usage? If that is the case see how I exported cpu-usage https://github.com/open-telemetry/opentelemetry-java/blob/master/contrib/src/main/java/io/opentelemetry/contrib/metrics/runtime/GarbageCollector.java, otherwise please explain me a bit more what you have in mind.

I am also a bit worried about the name "raw" measurements happy to hear other options.

@SergeyKanzhelev
Copy link
Member

Views are in SDK. Should we move this out of this milestone and defer for later?

@yurishkuro
Copy link
Member Author

What I mean by basic counter is something that has Inc(delta) method. The absolute value is completely meaningless for it

@bogdandrutu
Copy link
Member

@bogdandrutu
Copy link
Member

Via the API I pointed you, you create a Counter, then you get to the timeseries from there.

@tedsuo
Copy link
Contributor

tedsuo commented Jun 6, 2019

@yurishkuro I'm closing this issue for now, as it is not a blocker. Please reopen if this is still an issue.

@tedsuo tedsuo closed this as completed Jun 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API API related issues
Projects
None yet
Development

No branches or pull requests

5 participants