feat(sdk-metrics-base) Add pull controller #2517

Sobuno · 2021-10-01T19:17:54Z

Which problem is this PR solving?

Fixes #796

This PR introduces a Pull Controller. In contrast to the Push Controller which collects metrics at a fixed interval, this controller leaves the decision of when to collect to the MetricExporter. The implementation is based on @dyladan's comment here: #796 (comment). This kind of controller is great for e.g. a Prometheus exporter as Prometheus is generally pull-based (Except for their Pushgateway)

Short description of the changes

An optional function registerPullController can be defined on the MetricExporter. If defined, a PullController is created instead of a PushController. The MetricExporter must then itself call collect on the PullController to collect metrics when needed.

linux-foundation-easycla · 2021-10-01T19:17:57Z

The committers are authorized under a signed CLA.

✅ Sobuno (f734068, 3770da3)

dyladan · 2021-10-01T19:19:27Z

@Sobuno thanks for the contribution. I see you structured this based on a comment that I made, but I made that comment quite a while ago and the spec has changed a lot since then. Have you read the spec to ensure this change matches the latest state?

Please make sure to sign the CLA

Sobuno · 2021-10-01T19:30:11Z

@Sobuno thanks for the contribution. I see you structured this based on a comment that I made, but I made that comment quite a while ago and the spec has changed a lot since then. Have you read the spec to ensure this change matches the latest state?

Please make sure to sign the CLA

Per https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#metricexporter regarding Pull Metric Exporters: "Implementors MAY choose the best idiomatic design for their language." - the spec generally doesn't limit the design choices for Pull Metric Exporters, so I believe it should be in-spec.

I have signed the CLA now.

codecov · 2021-10-01T19:58:01Z

Codecov Report

Merging #2517 (a7b8bea) into main (d15127c) will increase coverage by 1.46%.
The diff coverage is n/a.

❗ Current head a7b8bea differs from pull request most recent head 3770da3. Consider uploading reports for the commit 3770da3 to get more accurate results

@@            Coverage Diff             @@
##             main    #2517      +/-   ##
==========================================
+ Coverage   93.23%   94.70%   +1.46%     
==========================================
  Files         137       66      -71     
  Lines        5042     1963    -3079     
  Branches     1066      434     -632     
==========================================
- Hits         4701     1859    -2842     
+ Misses        341      104     -237

Impacted Files	Coverage Δ
...telemetry-sdk-metrics-base/src/export/Processor.ts
...ckages/opentelemetry-sdk-metrics-base/src/types.ts
...strumentation/src/platform/node/instrumentation.ts
...ges/opentelemetry-instrumentation-http/src/http.ts
...ages/opentelemetry-api-metrics/src/types/Metric.ts
...ages/opentelemetry-exporter-otlp-http/src/types.ts
...ry-instrumentation-grpc/src/grpc-js/serverUtils.ts
...emetry-sdk-metrics-base/src/ValueRecorderMetric.ts
...ackages/opentelemetry-api-metrics/src/NoopMeter.ts
...opentelemetry-sdk-metrics-base/src/export/types.ts
... and 61 more

dyladan

I don't see any tests for this, nor do I see any documentation or examples of its use.

In general, I don't think it's a badly flawed idea, but I think the metric export pipeline in general would do well to have a design review while we still can and ensure it is flexible enough to meet our needs while being maintained long term. I'm not sure this is the route I would go in today. It might end up being what we do, but I think this is a good opportunity to explore other options as well.

dyladan · 2021-10-01T19:58:50Z

experimental/packages/opentelemetry-sdk-metrics-base/src/export/Controller.ts

+    return this.collect();
+  }
+
+  public async collect(): Promise<void> {


Why is collect public? I don't see it called anywhere. Is it expected to be called by exporters? If so, isn't it a little odd that calling the controller causes the controller to call the exporter? Seems like an unnecessarily convoluted export chain.

It was to keep implementing a push-based versus pull-based exporter very similar, only difference being the added function registerPullCollector and having the exporter invoke the collect function itself

Have you looked at implementations in other languages like python and go to see what they're doing?

Haven't really looked around much as I found your comment and figured that sounded pretty simple :) I will take a look around if I find the time.

I had a look at the Go implementation (https://github.com/open-telemetry/opentelemetry-go/blob/main/sdk/metric/controller/basic/controller.go)

They have a single Controller that has (among others) Start, Collect and ForEach exposed - Start starts the periodic calling of Collect whereas Collect itself collects the metrics and pushes them to the exporter (if one is defined), so basically the same pattern as above. ForEach "calls the passed function once per instrumentation library, allowing the caller to emit metrics grouped by the library that produced them."

They differ in that I do not see a way for the exporter to obtain a handle on the Collector (as I've done through the registerPullController function) - Indeed, the Go Prometheus Exporter explicitly states it does not implement the metric.Exporter interface, but instead itself creates a pull controller (https://github.com/open-telemetry/opentelemetry-go/blob/main/exporters/prometheus/prometheus.go#L41) and calls Collect, followed by ForEach whenever the HTTP endpoint is invoked.

If we were to follow their design more closely, we should have one Collector with public Start, Collect and ForEach methods. I do not see a simple way other than the aforementioned registerPullController to expose a handle to the Collector though - What the Go Prometheus Exporter does is basically set its internal Controller as the MetricsProvider, but we do not expose that as an option in the NodeSDKConfiguration.

Sobuno · 2021-10-01T20:20:07Z

Regarding examples, I was considering changing the Prometheus Exporter to use this - wasn't sure if you wanted that separated into another PR.

Wasn't sure on the testing policy for experimental packages - at least the linting command on the overall project (as listed in the contribution document mentioned when creating a PR) doesn't seem to run on these packages.

dyladan · 2021-10-01T20:21:29Z

Regarding examples, I was considering changing the Prometheus Exporter to use this - wasn't sure if you wanted that separated into another PR.

Wasn't sure on the testing policy for experimental packages - at least the linting command on the overall project (as listed in the contribution document mentioned when creating a PR) doesn't seem to run on these packages.

Splitting the experimental packages into their own directory was done only recently so some packages could be released as 1.0. The testing and linting requirements are not changed.

Thanks for letting me know the lint isnt running on the experimental packages. I'll look into it.

legendecas · 2021-10-03T16:51:21Z

Per https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#metricexporter regarding Pull Metric Exporters: "Implementors MAY choose the best idiomatic design for their language." - the spec generally doesn't limit the design choices for Pull Metric Exporters, so I believe it should be in-spec.

In the latest spec, there is a MetricsReader and they are responsible for deciding when to collect metrics. I'm wondering if it is better for us to align the interfaces with the spec since there are no controller things anymore in the spec.

Sobuno added 2 commits October 1, 2021 21:15

Add PullController

f734068

_collect to collect to follow naming standards

3770da3

Sobuno requested a review from a team October 1, 2021 19:17

dyladan reviewed Oct 1, 2021

View reviewed changes

Sobuno closed this Nov 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sdk-metrics-base) Add pull controller #2517

feat(sdk-metrics-base) Add pull controller #2517

Sobuno commented Oct 1, 2021 •

edited

Loading

linux-foundation-easycla bot commented Oct 1, 2021 •

edited

Loading

dyladan commented Oct 1, 2021

Sobuno commented Oct 1, 2021 •

edited

Loading

codecov bot commented Oct 1, 2021 •

edited

Loading

dyladan left a comment

dyladan Oct 1, 2021

Sobuno Oct 1, 2021

dyladan Oct 1, 2021

Sobuno Oct 1, 2021

Sobuno Oct 2, 2021

Sobuno commented Oct 1, 2021

dyladan commented Oct 1, 2021

legendecas commented Oct 3, 2021

feat(sdk-metrics-base) Add pull controller #2517

feat(sdk-metrics-base) Add pull controller #2517

Conversation

Sobuno commented Oct 1, 2021 • edited Loading

Which problem is this PR solving?

Short description of the changes

linux-foundation-easycla bot commented Oct 1, 2021 • edited Loading

dyladan commented Oct 1, 2021

Sobuno commented Oct 1, 2021 • edited Loading

codecov bot commented Oct 1, 2021 • edited Loading

Codecov Report

dyladan left a comment

Choose a reason for hiding this comment

dyladan Oct 1, 2021

Choose a reason for hiding this comment

Sobuno Oct 1, 2021

Choose a reason for hiding this comment

dyladan Oct 1, 2021

Choose a reason for hiding this comment

Sobuno Oct 1, 2021

Choose a reason for hiding this comment

Sobuno Oct 2, 2021

Choose a reason for hiding this comment

Sobuno commented Oct 1, 2021

dyladan commented Oct 1, 2021

legendecas commented Oct 3, 2021

Sobuno commented Oct 1, 2021 •

edited

Loading

linux-foundation-easycla bot commented Oct 1, 2021 •

edited

Loading

Sobuno commented Oct 1, 2021 •

edited

Loading

codecov bot commented Oct 1, 2021 •

edited

Loading