refactor(metrics): export pipeline #1017

legendecas · 2020-05-05T08:17:43Z

Metrics exporting requires a clear policy about when to export. This PR extracts controllers from metrics.Meter and makes it installable on metrics.MeterProvider.

Which problem is this PR solving?

MetricsProvider should act as a Controller to determine when to collect all metrics values.
Related Create a PullController for metrics #796.
Go impl: https://github.com/open-telemetry/opentelemetry-go/blob/a485d0ec64a48f6b7d5344790ba6c4b85f154e8e/sdk/metric/doc.go#L156

Short description of the changes

metrics.MeterProvider exposes a new method collect that similar to metrics.Meter.collect.
Update the PushController to accept a metrics.MeterProvider but not a metrics.Meter.
Add public installExportPipeline function to @opentelemetry/exporter-prometheus.
Add public installExportPipeline function to ConsoleExporter in @opentelemetry/metrics to install a push controller on global api.metrics.

packages/opentelemetry-metrics/src/export/controllers/PushController.ts

Metrics exporting requires a clear policy about when to export. While MetricsProvider holds all meters created, it can act as a Controller role to integrate better export strategies like pulling.

codecov-io · 2020-05-06T02:51:35Z

Codecov Report

Merging #1017 into master will increase coverage by 0.26%.
The diff coverage is 98.03%.

@@            Coverage Diff             @@
##           master    #1017      +/-   ##
==========================================
+ Coverage   93.04%   93.30%   +0.26%     
==========================================
  Files         137      140       +3     
  Lines        3794     3946     +152     
  Branches      784      808      +24     
==========================================
+ Hits         3530     3682     +152     
  Misses        264      264

Impacted Files	Coverage Δ
packages/opentelemetry-metrics/src/Meter.ts	`95.69% <ø> (-0.22%)`	⬇️
packages/opentelemetry-metrics/src/export/types.ts	`100.00% <ø> (ø)`
packages/opentelemetry-metrics/src/types.ts	`100.00% <ø> (ø)`
...y-metrics/src/export/controllers/PushController.ts	`94.11% <94.11%> (ø)`
...pentelemetry-exporter-prometheus/src/prometheus.ts	`90.74% <100.00%> (+0.84%)`	⬆️
...ackages/opentelemetry-metrics/src/MeterProvider.ts	`100.00% <100.00%> (ø)`
...emetry-metrics/src/export/ConsoleMetricExporter.ts	`75.00% <100.00%> (+12.50%)`	⬆️
...ntelemetry-metrics/src/export/controllers/index.ts	`100.00% <100.00%> (ø)`
...telemetry-plugin-fetch/src/enums/AttributeNames.ts	`100.00% <0.00%> (ø)`
packages/opentelemetry-plugin-fetch/src/fetch.ts	`96.52% <0.00%> (ø)`
... and 3 more

obecny

There are still unresolved few other issues:

Updating examples to use PushControler instead of MeterProvider
Update all README.md files (MeterProvider -> PushController)
There is as setGlobalMeterProvider what will happen with this, should this be renamed and refactored ?
We have NoopMeterProvider, what about this ?
In Api we still call it as MeterProvider but here is a PushController, which might be misleading, I think this should be unified either in one way or another.

obecny · 2020-05-06T11:48:38Z

packages/opentelemetry-metrics/src/index.ts

 export * from './Metric';
 export * from './MetricObservable';
 export * from './export/aggregators';
+export * from './export/controllers';


As you added index.ts into export folder I think this should be sufficient and below line
export * from './export/Controller'
should be a part of export/controllers/index.ts as well as
export * from './export/ConsoleMetricExporter'
WDYT ?

obecny · 2020-05-06T11:49:48Z

packages/opentelemetry-metrics/test/export/controllers/PushController.test.ts

  describe('constructor', () => {
    it('should construct an instance without any options', () => {
-      const provider = new MeterProvider();
-      assert.ok(provider instanceof MeterProvider);
+      const provider = new PushController();


Nit: is the variable name still suitable for provider , maybe now should be a controller ?

obecny · 2020-05-06T11:53:24Z

packages/opentelemetry-metrics/src/export/controllers/PushController.ts

@@ -0,0 +1,73 @@
+/*!


It would be nice to rename MeterProvider.ts into PushController.ts to preserve the history, as well as to avoid any merging conflicts in case there are changes from other PR at the same time. Exactly the same you did for tests

obecny · 2020-05-06T12:11:28Z

packages/opentelemetry-metrics/src/export/controllers/PushController.ts

+const DEFAULT_EXPORT_INTERVAL = 60_000;
+
+/**
+ * This class represents a meter provider collecting metric instrument values periodically.


update doc pls

mayurkale22 · 2020-05-06T18:33:32Z

I am little confused about the this PR (esp. MeterProvider -> PushController), is this something enforced by the specs?

Currently, Meter instance encapsulates the Controller logic. I think it should only be responsible for creating and recording metrics. And we should have separate export pipeline using either PushController or PullController.

Basically, users have to do the following things:

Create meter using MeterProvider
const meter = new MeterProvider();
Define the Contoller which will take meter and exporter to export the data at some interval.
const controller = new PushController(meter, exporter, interval);
OR
const controller = new PullController(meter, exporter);

mayurkale22 · 2020-05-06T18:58:22Z

Looks like this is similar to Python's implementation. See: https://github.com/open-telemetry/opentelemetry-python/blob/master/docs/examples/basic_meter/basic_metrics.py#L66

legendecas · 2020-05-08T07:25:27Z

@mayurkale22 an alternative could be the pattern opentelemetry-go is using:

(the example is written in JavaScript to illustrate possible API shapes)

import prometheus from '@opentelemetry/exporter-prometheus'
import { metrics } from '@opentelemetry/api'

prometheus.installPipeline(); // registers the PullController as globalMeterProvider.
const meter = metrics.getMeter('example-meter');
const counter = meter.createCounter('foo');
// ...

In this example, end users of @opentelemetry/api/@opentelemetry/metrics may not aware the controller/MeterProvider, these are taken care of by the exporters since those exporters do care about how the metrics are going to be collected.

The python example does work. But dangling a variable like pushController doesn't seem to be a best practice to me (they are ref-ed by hosts, cool, but confusing).

WDYT? @mayurkale22 @obecny

dyladan · 2020-05-08T13:57:34Z

so the exporter decides to install push or pull controller as the global meter provider?

vmarchaud · 2020-05-08T15:24:37Z

Woudn't it mean that one meter provider can only be pull or push based, so you wouldn't be able to use two exporters at the same time that use each method ?

legendecas · 2020-05-08T15:58:18Z

so the exporter decides to install push or pull controller as the global meter provider?

Yes, push or pull strategy is based on how the metrics are exported. So I think this is more intuitive.

Woudn't it mean that one meter provider can only be pull or push based, so you wouldn't be able to use two exporters at the same time that use each method ?

As a matter of fact, MeterProvider in @opentelemetry/metrics doesn't support multiple exporters already. Though the exporter can act like MultiSpanProcessor passing the metric records to all subsidiaries.

# Conflicts: # packages/opentelemetry-metrics/src/export/controllers/PullController.ts # packages/opentelemetry-metrics/test/export/controllers/PushController.test.ts

legendecas · 2020-05-31T12:05:38Z

@dyladan @mayurkale22 @obecny @vmarchaud hey, I've updated the PR to address the comments. Maybe there can be another round of review?

vmarchaud

overall lgtm, few questions though

vmarchaud · 2020-05-31T12:55:38Z

packages/opentelemetry-metrics/src/export/controllers/PushController.ts

+ * This class represents a controller collecting metric instrument values
+ * periodically.
+ */
+export class PushController extends PullController {


If some logic is shared between Push and Pull controller, could we move it to an abstract class BaseController and then make the PushController extending it instead of the PullController ?

packages/opentelemetry-metrics/README.md

vmarchaud · 2020-05-31T13:00:57Z

packages/opentelemetry-metrics/src/export/controllers/Controller.ts

+
+  protected onExportResult(result: ExportResult): void {
+    if (result !== ExportResult.SUCCESS) {
+      // @todo: log error


I think it's worth to add a logger option while are it, maybe as a optional parameter for now, WYTD ?

dyladan

This looks like really good work. Having now taken some time to go through it, I think I like this better than the current approach.

Minor issue: with the naming change, it may not be obvious to users that the controller is the meter provider.

packages/opentelemetry-metrics/src/export/controllers/Controller.ts

packages/opentelemetry-exporter-prometheus/src/prometheus.ts

packages/opentelemetry-metrics/src/export/controllers/PushController.ts

explicitly document the point

legendecas · 2020-07-06T06:29:54Z

@dyladan: Minor issue: with the naming change, it may not be obvious to users that the controller is the meter provider.

I've appended more ts-doc on controllers to explicitly indicate that those controllers are capable of been installed as the global metric provider.

obecny

The spec says about MeterProvider, why should we call it then as PullController then ?

obecny · 2020-07-06T14:52:49Z

packages/opentelemetry-exporter-prometheus/src/export/export-pipeline.ts

+ * limitations under the License.
+ */
+
+import * as api from '@opentelemetry/api';


the installExportPipeline for me it looks kind of magic from outside, not sure what is happening inside and how the exporter is being bound with meter and what if I want to change it. If we want to have similar pattern for other exporters (imho we should have those things unified) I would move installExportPipeline to a core or metrics and allow setting exporter as dependency injection. This way it will look a bit more clear for me and I will still be able to use the same pattern for any other exporter that I want. WDYT ?

The key point for installExportPipeline is that the export policy mainly depends on how the exporter works. For example, the prometheus exporter (which starts a server awaiting incoming scrapping) would prefer a pulling strategy and the console exporter would prefer a periodically print of the metric records. For this reason, the installExportPipeline was implemented for each exporter and in the installExportPipeline there is nothing magic other than installing a metric exporting strategy (controller) as the global metric provider as the metric provider is the role who holds all meters/metrics.

If the installExportPipeline is implemented as a method in opentelemetry/metric or core, there is no significant difference with the current approach since the exporting strategy still needs to be determined by who installs the pipeline which can be probably the end-user who uses the exporter rather than the exporter themselves.

e.g.

metrics.installExportPipeline({ exporter: new PrometheusExporter({ startServer: true }), strategy: 'pull' });

The assumption is based on that the end-user doesn't have to care about the strategy the exporter used. They do care about which exporter they are going to work with.

obecny · 2020-07-06T15:00:08Z

packages/opentelemetry-metrics/README.md

+import { metrics } from '@opentelemetry/api'
+
+// Install the export pipeline before all subsequent call to metrics.
+ConsoleMetricExporter.installPipeline();


why is this static, have different name than in prometheus and the flow will also looks differently ?
I think this all should look the same doesn't matter if this is console exporter, prometheus exporter or any other exporter. I think we should have then one method in metrics which will install the export pipeline by accepting an exporter as dependency injection.

I'll rename those installExportPipeline aligned if the above explanation make sense to you.

dyladan · 2020-07-06T16:30:20Z

Maybe we can resolve naming confusion by naming the controllers PullControllerMeterProvider and PushControllerMeterProvider.

obecny · 2020-07-06T16:41:57Z

Maybe we can resolve naming confusion by naming the controllers PullControllerMeterProvider and PushControllerMeterProvider.

I would keep the naming the same as in spec to avoid any confusion (MeterProvider) but then maybe some option can decide if that is a push or pull strategy ?.

const provider = new MeterProvider('pull'); // default to 'push' as it was
const provider = new MeterProvider({controller: 'pull'}); // default to 'push' as it was
const provider = new MeterProvider({strategy: 'pull'}); // default to 'push' as it was
...

dyladan · 2020-07-15T13:43:38Z

What is the status of this? It has 3 reviews, but open comments from a maintainer.

@obecny you are the only one with remaining open comments here.

obecny · 2020-07-15T14:03:21Z

What is the status of this? It has 3 reviews, but open comments from a maintainer.

@obecny you are the only one with remaining open comments here.

My main concern is that why this is done different then what spec says. Spec says about MeterProvider, not PushController.
I think @mayurkale22 had the same concerns.

legendecas · 2020-07-15T17:57:30Z

@obecny I've updated the PR the revert the changes made to MeterProvider naming.

Since this update, the PR can be summarized as:

metrics.MeterProvider exposes a new method collect that similar to metrics.Meter.collect, collecting all metrics of all meters that the meter provider creates.
Renamed Controller to PushController, and the new PushController accepts a metrics.MeterProvider but not a metrics.Meter.
Add public installExportPipeline function to @opentelemetry/exporter-prometheus.
Add public installExportPipeline function to ConsoleExporter in @opentelemetry/metrics to install a push controller on global api.metrics.

Example usage of the new pipeline:

import { ConsoleMetricExporter } from '@opentelemetry/metrics'
import { metrics } from '@opentelemetry/api'

// Install the export pipeline before all subsequent call to metrics.
ConsoleMetricExporter.installExportPipeline();
// Identical to:
(function installExportPipeline() {
  const exporter = new ConsoleMetricExporter();
  const meterProvider = new MeterProvider({ exporter });
  const controller = new PushController(meterProvider);
  api.metrics.setGlobalMeterProvider(meterProvider);
})();

const meter = metrics.getMeter('example-meter');
const counter = meter.createCounter('foo');

obecny

it looks great now, just few smaller things with return type

packages/opentelemetry-exporter-prometheus/src/prometheus.ts

packages/opentelemetry-metrics/src/MeterProvider.ts

obecny · 2020-07-15T19:07:32Z

packages/opentelemetry-metrics/src/export/ConsoleMetricExporter.ts

+  static installExportPipeline(
+    config?: PushControllerConfig,
+    meterConfig?: MeterConfig
+  ) {


missing return type - I think it should be the same interface that you have in prometheus, we might use it later for CollectorMetricExporter as well

packages/opentelemetry-metrics/test/MeterProvider.test.ts

dyladan · 2020-07-17T20:13:59Z

First, thank you for the work you have put into this. I know this has been a lot and the review process has been lengthy, but this is a large PR and there is a lot to unpack here. First, I have a couple of questions.

Why did you decide to go with this model, rather than the simpler route of adding a pull controller to the existing model? This is a large change from what we previously had. A smaller change would be something like this:

interface PullingExporter extends Exporter {
  registerPullController(controller: PullController): void;
}


const pullingExporter = new Exporter1(); // implements pulling exporter
const pushingExporter = new Exporter2(); // implement exporter

const provider = new MeterProvider();

// option a
provider.addController(new PullController(pullingExporter));
provider.addController(new PushController(pushingExporter);

/// OR

// option b
provider.addPullExporter(pullingExporter);
provider.addExporter(pushingExporter);

provider.register();

In addition to being simpler, this would also have good symmetry with the tracing API. Between option a and b I wouldn't have a strong opinion.

This looks similar to the go and python implementations with one notable exception. As you can see here, the export pipeline installation method is on the MeterProvider in Python, where you decided to put it in the exporter. Can you please explain why you decided to add the function to the exporter rather than the provider? I can easily see a situation where the user writes the following code and is confused why it doesn't work:

const provider = new MeterProvider();
metrics.setMeterProvider(provider);

const meter = provider.getMeter("myapp");

const exporter = new Exporter();
exporter.installExportPipeline();

const counter = meter.createCounter();

// use counter in some way, metrics are not exported, user is confused

Because this PR has the installExportPipeline create a new MeterProvider, the user's counter in this case would not work with the export pipeline and the user would see no metrics. In some ways, this is a documentation issue, but we should also do our best to avoid situations like this where it is so easy to misuse the API.

legendecas · 2020-07-20T18:01:24Z

@dyladan Thanks for the questions.

The idea behind the metricExporter.installExportPipeline is that metric exporter has to decide their metric collection frequency to reduce costs (reduce proactive metric collections like system resources utilization) at their best efforts. My experience is some APM vendors do only collect metrics with a limit set of time periods, like 5s, or 15s. If the pipeline is installed by users, like the python example, the users have to decide the intervals of metric collections. In that way, the exporter may have to implement their own caching mechanism to not flushing their servers if the user sets quite a short interval. However, if the exports do support arbitrary intervals (or strategies), they can provide options to alter that if the installExportPipeline is implemented by the exporters.

However, I found I'm fallen into a thought trap while reviewing the first question. My initial thought is that we can lift the decision like the strategy the exporter should use. Yet I found the symmetry between the metrics and tracing you mentioned is interesting idea on how the metrics export pipeline should be defined, regardless of how currently go or python SDK is implemented.

My thoughts on the symmetry can be: Aggregator & Controller <-> Sampler. Which aggregator to use is decided by the batcher, which is installed at the construction of MeterProvider. Similarly, samplers will be installed on the construction of BasicTracerProvider. So for controllers, I'm wondering if the current approach is the right way to keep symmetry between metrics and tracing. I'm opening to revert the current changes to only migrating Controllers on the main branch to bind with MeterProvider rather than single Meter instance (so that this PR can be split into smaller ones), and continue the export pipeline installation discussion in another PR (or issue).

legendecas · 2020-09-13T17:04:43Z

The export pipeline user interface may be hard to be a stable design as the metric record export pipeline of metrics SDK specification is still been drafting actively. Closed now and keep tracking on #1528.

…ts (open-telemetry#1017) * fix: remove cassandra types from exports * fix: remove router types from exports * fix: remove knex types from exports * fix: remove mysql2 types from exports * style: lint mysql2 source * fix: keep `typeof Router` in non-public API

legendecas requested review from bg451, dyladan, markwolff, mayurkale22, obecny, OlivierAlbertini, rochdev and vmarchaud as code owners May 5, 2020 08:17

legendecas force-pushed the pull-controller branch from 3f48a87 to 8f1e7d8 Compare May 5, 2020 08:22

legendecas mentioned this pull request May 5, 2020

Create a PullController for metrics #796

Closed

vmarchaud reviewed May 5, 2020

View reviewed changes

packages/opentelemetry-metrics/src/export/controllers/PushController.ts Show resolved Hide resolved

refactor(metrics): metrics provider implements controller

af75f12

Metrics exporting requires a clear policy about when to export. While MetricsProvider holds all meters created, it can act as a Controller role to integrate better export strategies like pulling.

legendecas force-pushed the pull-controller branch from 8f1e7d8 to af75f12 Compare May 6, 2020 02:48

obecny reviewed May 6, 2020

View reviewed changes

legendecas added 4 commits May 24, 2020 22:10

refactor: make PushController a variant of PullController

963fb8c

Merge branch 'master' into pull-controller

5ada775

# Conflicts: # packages/opentelemetry-metrics/src/export/controllers/PullController.ts # packages/opentelemetry-metrics/test/export/controllers/PushController.test.ts

feat: add server start callback

e80e541

feat: prometheus export pipeline

e7afc5f

legendecas requested a review from mwear as a code owner May 24, 2020 15:14

legendecas added 2 commits May 28, 2020 23:43

Merge branch 'master' into pull-controller

4b22cfb

Merge branch 'master' into pull-controller

60af018

vmarchaud reviewed May 31, 2020

View reviewed changes

vmarchaud mentioned this pull request Jun 24, 2020

feat(prom): expose measure as summary #982 #1038

Closed

dyladan reviewed Jun 24, 2020

View reviewed changes

legendecas added 4 commits July 3, 2020 17:18

Merge branch 'master' into pull-controller

c88bcec

test: await asynchronous collect

58eddf2

docs: controllers can be installed as global meter provider

afbf0b2

explicitly document the point

feat(prometheus): add support for async metric collection

d4a45bc

dyladan approved these changes Jul 6, 2020

View reviewed changes

obecny reviewed Jul 6, 2020

View reviewed changes

markwolff approved these changes Jul 6, 2020

View reviewed changes

legendecas added 2 commits July 16, 2020 01:01

Merge remote-tracking branch 'upstream/master' into pull-controller

b679073

refactor: fix naming confusion issue

8440c53

legendecas changed the title ~~refactor(metrics): metrics provider implements controller~~ refactor(metrics): export pipeline Jul 15, 2020

style: autofix

30e883b

obecny reviewed Jul 15, 2020

View reviewed changes

legendecas added 2 commits July 16, 2020 12:18

feat: add export pipeline installer interface

65c2e46

Merge remote-tracking branch 'upstream/master' into pull-controller

87a5981

legendecas mentioned this pull request Jul 21, 2020

feat: bind controller with wrapped meter provider #1333

Closed

legendecas mentioned this pull request Sep 13, 2020

[Metrics] Metrics SDK export pipeline tracking issue #1528

Closed

legendecas closed this Sep 13, 2020

legendecas deleted the pull-controller branch December 14, 2021 08:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(metrics): export pipeline #1017

refactor(metrics): export pipeline #1017

legendecas commented May 5, 2020 •

edited

Loading

codecov-io commented May 6, 2020 •

edited by codecov bot

Loading

obecny left a comment

obecny May 6, 2020

obecny May 6, 2020

obecny May 6, 2020

obecny May 6, 2020

mayurkale22 commented May 6, 2020

mayurkale22 commented May 6, 2020

legendecas commented May 8, 2020

dyladan commented May 8, 2020

vmarchaud commented May 8, 2020

legendecas commented May 8, 2020

legendecas commented May 31, 2020

vmarchaud left a comment

vmarchaud May 31, 2020

vmarchaud May 31, 2020

dyladan left a comment

legendecas commented Jul 6, 2020

obecny left a comment

obecny Jul 6, 2020

legendecas Jul 7, 2020

obecny Jul 6, 2020

legendecas Jul 7, 2020 •

edited

Loading

dyladan commented Jul 6, 2020

obecny commented Jul 6, 2020

dyladan commented Jul 15, 2020

obecny commented Jul 15, 2020

legendecas commented Jul 15, 2020 •

edited

Loading

obecny left a comment

obecny Jul 15, 2020

dyladan commented Jul 17, 2020 •

edited

Loading

legendecas commented Jul 20, 2020

legendecas commented Sep 13, 2020

refactor(metrics): export pipeline #1017

refactor(metrics): export pipeline #1017

Conversation

legendecas commented May 5, 2020 • edited Loading

Which problem is this PR solving?

Short description of the changes

codecov-io commented May 6, 2020 • edited by codecov bot Loading

Codecov Report

obecny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayurkale22 commented May 6, 2020

mayurkale22 commented May 6, 2020

legendecas commented May 8, 2020

dyladan commented May 8, 2020

vmarchaud commented May 8, 2020

legendecas commented May 8, 2020

legendecas commented May 31, 2020

vmarchaud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyladan left a comment

Choose a reason for hiding this comment

legendecas commented Jul 6, 2020

obecny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legendecas Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

dyladan commented Jul 6, 2020

obecny commented Jul 6, 2020

dyladan commented Jul 15, 2020

obecny commented Jul 15, 2020

legendecas commented Jul 15, 2020 • edited Loading

obecny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyladan commented Jul 17, 2020 • edited Loading

legendecas commented Jul 20, 2020

legendecas commented Sep 13, 2020

legendecas commented May 5, 2020 •

edited

Loading

codecov-io commented May 6, 2020 •

edited by codecov bot

Loading

legendecas Jul 7, 2020 •

edited

Loading

legendecas commented Jul 15, 2020 •

edited

Loading

dyladan commented Jul 17, 2020 •

edited

Loading