refactor: change metrics export data model to match OTLP protos #2809

seemk · 2022-02-28T20:28:57Z

Which problem is this PR solving?

Refactor metrics export data model to be more in line with metrics protos.

Short description of the changes

This introduces MetricsData (naming suggestions welcome) class, which internally groups metric datapoints by resource and instrumentation library and provides a helper to "flatten" into the OTLP protos shape.

Thus instrumentationLibrary and resource are removed from BaseMetricData, saving memory and code duplication.

Checklist:

Followed the style guidelines of this project
Unit tests have been added

codecov · 2022-02-28T20:30:33Z

Codecov Report

Merging #2809 (4411266) into main (82192b5) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2809      +/-   ##
==========================================
- Coverage   93.48%   93.46%   -0.02%     
==========================================
  Files         163      163              
  Lines        5541     5543       +2     
  Branches     1166     1167       +1     
==========================================
+ Hits         5180     5181       +1     
- Misses        361      362       +1

Impacted Files	Coverage Δ
...s-base/src/export/PeriodicExportingMetricReader.ts	`95.34% <0.00%> (-2.22%)`	⬇️
...ckages/opentelemetry-sdk-metrics-base/src/Meter.ts	`97.82% <0.00%> (ø)`
...entelemetry-sdk-metrics-base/src/aggregator/Sum.ts	`100.00% <0.00%> (ø)`
...ntelemetry-sdk-metrics-base/src/aggregator/Drop.ts	`100.00% <0.00%> (ø)`
...telemetry-sdk-metrics-base/src/aggregator/types.ts	`100.00% <0.00%> (ø)`
...elemetry-sdk-metrics-base/src/export/MetricData.ts	`100.00% <0.00%> (ø)`
...emetry-sdk-metrics-base/src/export/MetricReader.ts	`100.00% <0.00%> (ø)`
...emetry-sdk-metrics-base/src/state/MetricStorage.ts	`100.00% <0.00%> (ø)`
...metry-sdk-metrics-base/src/aggregator/Histogram.ts	`97.91% <0.00%> (ø)`
...metry-sdk-metrics-base/src/aggregator/LastValue.ts	`100.00% <0.00%> (ø)`
... and 4 more

legendecas

Thank you for your work! 👍

legendecas · 2022-03-01T02:38:03Z

experimental/packages/opentelemetry-sdk-metrics-base/src/Meter.ts

@@ -130,16 +130,21 @@ export class Meter implements metrics.Meter {
   * @param collectionTime the HrTime at which the collection was initiated.
   * @returns the list of {@link MetricData} collected.
   */
-  async collect(collector: MetricCollectorHandle, collectionTime: HrTime): Promise<MetricData[]> {
-    const result = await Promise.all(Array.from(this._metricStorageRegistry.values()).map(metricStorage => {
+  async collect(collector: MetricCollectorHandle, collectionTime: HrTime): Promise<MetricsData> {


It seems like InstrumentationLibraryMetrics is what a Meter represents regarding the export model. Maybe we can return a Promise<InstrumentationLibraryMetrics> here?

Good point, even less code :) Changed it

legendecas · 2022-03-01T02:43:41Z

experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricCollector.ts

    const collectionTime = hrTime();
    const results = await Promise.all(this._sharedState.meters
      .map(meter => meter.collect(this, collectionTime)));

-    return results.reduce((cumulation, current) => cumulation.concat(current), []);
+    return results.reduce((cumulation, current) => cumulation.merge(current), new MetricsData());


When the meter returns a plain interface of InstrumentationLibraryMetricsData, it seems we don't need a MetricsData.merge here and exporters don't need MetricsData.resourceMetrics to transform maps of data to plain structures?

i.e. a ResourceMetrics could be constructed with an array of InstrumentationLibraryMetricsData and returned.

Thanks! This made it possible to get rid of MetricsData

ehudsh · 2022-03-01T16:27:46Z

Will this MR will make sdk-metrics-base to be compatible with OTLP proto 0.11+ ? Thanks !

legendecas · 2022-03-02T03:48:39Z

Will this MR will make sdk-metrics-base to be compatible with OTLP proto 0.11+ ? Thanks !

No, we still need to update the metrics-otlp exporter to adopt the changes.

seemk · 2022-03-04T15:08:26Z

Perhaps one open question - should metrics data be grouped together here by instrumentation library metrics (need to check for value equality) when doing the collect? Or do we think it's fine to have possible duplicates of instrumentation libraries there?

dyladan · 2022-03-04T15:15:26Z

It's technically fine to have duplicates there. Grouping them may have some small benefit on the total payload size

legendecas · 2022-03-04T15:38:36Z

Perhaps one open question - should metrics data be grouped together here by instrumentation library metrics (need to check for value equality) when doing the collect?

I think we can do it by preventing from creating duplicated meters, so that the burden is at the time creating the meter, rather than duplicating the work at the collection time. Either way, I think we don't need to do it in this PR. We can track the meter identity in #2593

legendecas

Overall LGTM 👍 , just one minor question.

legendecas · 2022-03-04T15:46:04Z

experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricCollector.ts

-    const results = await Promise.all(this._sharedState.meters
-      .map(meter => meter.collect(this, collectionTime)));
+    const instrumentationLibraryMetrics = (await Promise.all(this._sharedState.meters
+      .map(meter => meter.collect(this, collectionTime)))).filter(({ metrics }) => metrics.length > 0);


I'd find we don't need to do this filtering in the SDK. Like, say, with the DELTA aggregation temporality, the metrics for a meter may contain no items if in a period of time there is no metric event reported. It may be confusing that the InstrumentationLibrary sometimes disappears.

So you are suggesting to export empty InstrumentationLibraryMetrics in that case and leave it up to the exporter and/or backend to sort it out?

Yes, exactly.

Removed the filtering

dyladan

Looks good generally. Think this will be ready to merge soon

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

...rimental/packages/opentelemetry-sdk-metrics-base/src/export/PeriodicExportingMetricReader.ts

…hape

…try-js into metrics-sdk-batch-shape

vmarchaud

lgtm

legendecas

Thank you for your work!

seemk added 2 commits February 28, 2022 22:10

refactor: match metrics export model with proto

aace569

Merge branch 'main' into metrics-sdk-batch-shape

15bf092

seemk requested a review from a team February 28, 2022 20:28

legendecas reviewed Mar 1, 2022

View reviewed changes

seemk added 2 commits March 4, 2022 17:01

refactor: get rid of MetricsData

a8be4f9

Merge branch 'main' into metrics-sdk-batch-shape

1906892

legendecas reviewed Mar 4, 2022

View reviewed changes

dyladan reviewed Mar 4, 2022

View reviewed changes

experimental/packages/opentelemetry-sdk-metrics-base/src/export/MetricReader.ts Show resolved Hide resolved

...rimental/packages/opentelemetry-sdk-metrics-base/src/export/PeriodicExportingMetricReader.ts Outdated Show resolved Hide resolved

seemk added 3 commits March 4, 2022 20:14

Merge remote-tracking branch 'upstream/main' into metrics-sdk-batch-s…

11b24a7

…hape

Merge branch 'metrics-sdk-batch-shape' of github.com:seemk/openteleme…

a5c3ad3

…try-js into metrics-sdk-batch-shape

fix: don't filter empty instrumentation library metrics

a6b1bea

dyladan approved these changes Mar 4, 2022

View reviewed changes

vmarchaud approved these changes Mar 5, 2022

View reviewed changes

vmarchaud requested a review from legendecas March 5, 2022 08:53

Merge branch 'main' into metrics-sdk-batch-shape

4411266

legendecas approved these changes Mar 6, 2022

View reviewed changes

legendecas merged commit c6dab2a into open-telemetry:main Mar 6, 2022

aabmass mentioned this pull request Mar 18, 2022

Support histogram metrics in @google-cloud/opentelemetry-cloud-monitoring-exporter GoogleCloudPlatform/opentelemetry-operations-js#381

Closed

srikanthccv mentioned this pull request Apr 28, 2022

Refactor Metric object format open-telemetry/opentelemetry-python#2646

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: change metrics export data model to match OTLP protos #2809

refactor: change metrics export data model to match OTLP protos #2809

seemk commented Feb 28, 2022

codecov bot commented Feb 28, 2022 •

edited

Loading

legendecas left a comment

legendecas Mar 1, 2022

seemk Mar 4, 2022

legendecas Mar 1, 2022

seemk Mar 4, 2022

ehudsh commented Mar 1, 2022

legendecas commented Mar 2, 2022

seemk commented Mar 4, 2022

dyladan commented Mar 4, 2022

legendecas commented Mar 4, 2022

legendecas left a comment

legendecas Mar 4, 2022

dyladan Mar 4, 2022

legendecas Mar 4, 2022

seemk Mar 4, 2022

dyladan left a comment

vmarchaud left a comment

legendecas left a comment

refactor: change metrics export data model to match OTLP protos #2809

refactor: change metrics export data model to match OTLP protos #2809

Conversation

seemk commented Feb 28, 2022

Which problem is this PR solving?

Short description of the changes

Checklist:

codecov bot commented Feb 28, 2022 • edited Loading

Codecov Report

legendecas 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

ehudsh commented Mar 1, 2022

legendecas commented Mar 2, 2022

seemk commented Mar 4, 2022

dyladan commented Mar 4, 2022

legendecas commented Mar 4, 2022

legendecas 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

dyladan left a comment

Choose a reason for hiding this comment

vmarchaud left a comment

Choose a reason for hiding this comment

legendecas left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 28, 2022 •

edited

Loading