Metrics SDK spec skeleton #1673
Conversation
specification/metrics/sdk.md
Outdated
|
|
||
| ## MeasurementProcessor | ||
|
|
||
| `MeasurementProcessor` is an interface which allows hooks when a Measurement is |
There was a problem hiding this comment.
Are Aggregators expected to be implemented as MeasurementProcessor?
There was a problem hiding this comment.
At this stage we can implement aggregators as SDK internal components that can be used in a MeasurementProcessor, and we have better understanding, we can decide if we want to expose Aggregator as an interface.
specification/metrics/sdk.md
Outdated
| +------------------+ | ||
| | MeterProvider | +----------------------+ | ||
| | Meter A | Measurements... | | Metrics... +-----------------+ | ||
| | Instrument X |-----------------> MeasurementProcessor +------------> | |
There was a problem hiding this comment.
@jkwatson mentioned during the SIG meeting that we need a way to make composite processors.
There was a problem hiding this comment.
Related to this
There are a few different aspects of concern for me as it relates to both the MetricProcessors and Measurement Processors.
I have a few questions:
Would we want to allow for "parallel processes" on a singular stream? Or more clearly maybe is there a possibility that a split may occur where two separate processors operate on a measurement/metric to then join back later to be combined.
Are separate measurements coming in allowed to be joined into a single metric?
There was a problem hiding this comment.
Inside the SDK, the data will not join back.
It is possible that the data get forked and sent via multiple exporters (e.g. one exporter push at every 1 second, another export push every 1 minute), and the exporters send data to the same collector/backend. In this case the data might join back and cause problem.
There was a problem hiding this comment.
@hdost The data model specifies what "overlapping streams" look like. You could consider the parallel streams of your example intentionally overlapping points.
We may want to explain that the kind of parallel data you are describing should be renamed as a new metric name. I do not want to have to specify anything about parallel streams of data that are aggregated differently but carry duplicate meaning, for example.
reyang
added
area:sdk
| +------------------+ | ||
| ``` | ||
|
|
||
| ## MetricProcessor |
There was a problem hiding this comment.
Your current pipeline is...
[API/SDK] -> measurement -> [1. MeasurementProcessor] -> [2. MetricProcessor <-> [**In-Memory-State**]] -> metric -> [3. MetricExporter]
Maybe we can fully expand to...
[API/SDK] -> measurement -> [1. MeasurementProcessor]* -> measurement -> [2. MetricProcessor <-> [**In-Memory-State**]] -> [MetricCollector] -> metric -> [MetricEnricher] -> metric -> [3. MetricExporter]
MeasurementProcessor: Allows adjustment to attributes. Can be chained/composed.
MetricCollector: Collect aggregated metrics from In-Memory-State. This is likely build-in/internal to SDK.
MetricEnricher: Allows custom adjustment to metrics. Can be chained/composed.
There was a problem hiding this comment.
This is something I intended to avoid 😃
I think the goal is to start from a small set of essential (ideally must-have) LEGO pieces, and depending on what we've learned while building the SDKs we can decide to introduce more if there is a strong reason.
There was a problem hiding this comment.
+1 on avoiding this kind of strictness until there's a strong reason (thinking about mistakes I've made) 🤣
| Push Metric Exporter sends the data on its own schedule. Here are some examples: | ||
|
|
||
| * Sends the data based on a user configured schedule, e.g. every 1 minute. | ||
| * Sends the data when there is a severe error. |
There was a problem hiding this comment.
I like that this is declared as a use case.
| protocol-dependent telemetry exporters. The protocol exporter is expected to be | ||
| primarily a simple telemetry data encoder and transmitter. | ||
|
|
||
| Metric Exporter has access to the [pre-aggregated metrics |
There was a problem hiding this comment.
Can you clarify a bit on how Metric Exporter gain access to pre-aggregated metrics? Is it that it has access to the "in-memory state" or is it passed those via the MetricProcessor?
What is the "input" to MetricExporter? Is it metrics (pre/post aggregation?) or is it some batch / "exportable representation" format?
It may be helpful to label the arrows (like you did for input to in-memory state) between In-memory state, MetricProcessor, and MetricExporter.
There was a problem hiding this comment.
I've updated the diagram to clarify that exporters don't have direct access to the "in-memory-state", and it receives (whether pulls from, or being pushed) pre-aggregated metrics from the processor.
There was a problem hiding this comment.
The "input" should be a batch of "exportable representation", similar to the tracing SDK spec https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#exportbatch. These are the meats that we'll need to add in subsequent PRs.
|
@reyang Changelog please :-) |
reyang
force-pushed
the
reyang/metrics-sdk
branch
from
06d7869 to
6be59bd
Compare
|
@arminru I've been struggling if I should add changelog right now or later (e.g. when we remove the banner "language clients should not start the implementation unless we explicitly communicated"). If we put the changelog now, it might be too noisy? (I'm thinking that we have one changelog entry at late May saying "Added the experimental version of Metrics SDK Spec (a long list of PRs...)") Please let me know your suggestion. |
|
Looks like the PR is OK to merge - enough approvals, cool off period completed, and comments were addressed. |
* skeleton of metrics SDK spec * add more design questions * finish the skeleton * fix CI * add more links and clarification * nits * improve the wording for contex/baggage/trace interaction with metrics * fix typo * update the diagram to show logical in-memory state * update the diagram * update the wording * update the wording Co-authored-by: Sergey Kanzhelev <S.Kanzhelev@live.com>
Changes
Added the metrics SDK specification skeleton
Related issues #1116
Related oteps OTEP146