Add more details to SDK MeterProvider and View #1730
Conversation
reyang
added
area:sdk
There was a problem hiding this comment.
My concern here is the following:
- View is defined in the SDK as it's mostly a processing piepline thing, however it's really feeling like an APi thing
- I think there's a bifurcation of API that needs to happen as we specify between "How a user specified a view" and "The interface and processing of that view". I'm not sure I can tell where the line between the two is in this specification.
- Fundamentally, with Views, we're defining a sort of "query" language against metrics (prometheus rewrite rules e.g.). I think we can do something very simple to start with, but I know we'll get pushed this direction. We should set ourselves up for success there. i'd like to see more examples of "configuration" for views.
Do we have a prototype for these that I missed? (sorry the metric API meeting has been at a really inconvenient time for me in general).
specification/metrics/sdk.md
Outdated
| The MeterProvider MUST provide a way to register Views, and here are the | ||
| inputs: | ||
|
|
||
| * The Instrument selection criteria (required), which MUST cover: |
There was a problem hiding this comment.
From a 'raw level SDK' this seems fine, but as specified i think you NEED a helper method on top of this.
E.g. Can the user just glob-match on Version + Kind to get measurements based on Meter/Instrument-name?
I'm worried the way this language reads right now. I think it makes sense to tease apart "What SDK implementors of metric processors need" and "What SDK Users defining views need". does it make sense to split these two?
Effectively for users, your'e giving them a 'query' language to select metrics.
For the SDK, you're defining a data model "index" that needs to allow rapid processing.
i agree the 4 things you list are what belong on the index.
There was a problem hiding this comment.
This is missing something to select "Aggregation". "Customize the aggregation" is a listed use-case.
There was a problem hiding this comment.
I don't think this portion of the API needs to specify aggregation. Instead we do need to clarify if any selected instrument has its default aggregation removed, or if it still exists. It's unclear what would happen if I were, e.g. to do a "Select all" here for my metrics. Do they all get removed?
|
Based on discussion in the metrics API SiG, updating my comments as follows:
|
reyang
force-pushed
the
reyang/metrics-sdk-view
branch
from
07c6b40 to
be9beb5
Compare
| * The `extra dimensions` which come from Baggage/Context (optional). If not | ||
| provided, no extra dimension will be used. Please note that this only | ||
| applies to [synchronous Instruments](./api.md#synchronous-instrument). |
There was a problem hiding this comment.
I wonder if we should leave these out of the first pass. Otherwise, we need to figure out some standard way to figure out how to define the source of the information declaratively.
There was a problem hiding this comment.
What do you mean by "define the source of the information"?
There was a problem hiding this comment.
I think having this would help us to move forward (once we have the skeleton of the spec, it is easier for others to come and contribute the "meat") 😄
There was a problem hiding this comment.
I mean... how do you extract it from the context? Are you providing some sort of function on the context that would give you an attribute?
There was a problem hiding this comment.
My understanding is that the SDK via its MeasurementProcessor has access to the context for this purpose.
There was a problem hiding this comment.
sure...but how do you define a view to do that work? how do you specify how to extract data from the context?
|
@yurishkuro this pr has enough approvals (and discussion during SiG) to merge. |
Co-authored-by: John Watson <[email protected]>
|
Discussed during the Metrics SIG meeting, I've removed the histogram details, we should be ready to merge the PR @jsuereth. |
* Add View API * fix link * address comments * provide one example * update wording * polish wording * update wording * mention histogram bucket in the view aggregation * resolve comments based on discussion during the SIG meeting * fix typo * update the spec based on discussion during the SIG meeting * add more diagrams * update the spec based on the discussion during Metrics SIG meeting * minor tweak diagram * address review comment * update the default histogram buckets * remove pipeline from this PR * address comments regarding error handling * Update specification/metrics/sdk.md Co-authored-by: John Watson <[email protected]> * adjust wording * add instrument type and clarify the selection criteria * update the view selection logic * update the view selection logic * fix broken link * update the TODO part by removing the details Co-authored-by: John Watson <[email protected]>
Follow up #1673
Fixes #466
Related issues #1116
Changes
Related oteps OTEP146