feat(exporter-metrics-otlp-http): add option to set the exporter aggregation preference #4409

AkselAllas · 2024-01-10T15:18:54Z

Context is here

Should probably get some tests.

linux-foundation-easycla · 2024-01-10T15:18:58Z

The committers listed above are authorized under a signed CLA.

✅ login: AkselAllas / name: Aksel Allas (590392d, 6072a65)

AkselAllas · 2024-01-12T10:30:40Z

@pichlermarc Can you review this and advise on tests perhaps?

pichlermarc

Thanks for tackling this @AkselAllas
In it's current state, I think this might not yet work, fortunately it'll be easy to fix (see comment) 🙂

For tests, just having a few tests in this file, which tests if the aggregationSelector calls the selector supplied to the constructor, and returns it's results should be sufficient. A further test to check that it returns the default aggregation preference when nothing is supplied would also be nice. 🙂

pichlermarc · 2024-01-12T13:28:51Z

experimental/packages/opentelemetry-exporter-metrics-otlp-http/src/OTLPMetricExporterBase.ts

+    if (config?.aggregationSelector) {
+     this._otlpExporter.aggregationSelector = config.aggregationSelector
+    }


This sets the selector to the internal exporter, so It won't be used.
We'd have to do something like here where we implement selectAggregation and then call the selector from the config.

Seems that the necessary line existed already

Yep that's in the PeriodicExportingMetricReader accessing any PushMetricExporter that's passed in (which, in this case is an instance of the class in this file). However this code here sets it on_otlpExporter (which is just a field of this class that is used internally), so it's never picked up in the PeriodicExportingMetricReader

Sorry, I should have had that comment as draft :) I was already fixing that part of the missing code.

AkselAllas · 2024-01-17T13:43:21Z

Running npm run test doesn't work locally for me 🤔

Error: Cannot find module '../version'

Any ideas?

codecov · 2024-01-17T17:02:23Z

Codecov Report

Merging #4409 (6072a65) into main (828f2ed) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4409   +/-   ##
=======================================
  Coverage   92.23%   92.23%           
=======================================
  Files         336      336           
  Lines        9525     9533    +8     
  Branches     2020     2022    +2     
=======================================
+ Hits         8785     8793    +8     
  Misses        740      740

Files	Coverage Δ
...er-metrics-otlp-http/src/OTLPMetricExporterBase.ts	`94.11% <100.00%> (+0.78%)`	⬆️
...metrics-otlp-http/src/OTLPMetricExporterOptions.ts	`100.00% <ø> (ø)`

experimental/packages/opentelemetry-exporter-metrics-otlp-http/src/OTLPMetricExporterBase.ts

trentm · 2024-01-17T22:45:36Z

Error: Cannot find module '../version'

That file is usually generated as part of npm run compile (in the package subdir or at the top-level of the repo clone). To just update/generate the version.ts file: npm run version:update.

AkselAllas · 2024-01-18T06:12:45Z

Again local tests failed :(
Seemingly no actionable error though

AkselAllas · 2024-01-22T12:17:58Z

@pichlermarc Is there anything to improve here? Or is there something we are waiting after?

If merged, any idea when a release could be expected? Along with next release of @opentelemetry/instrumentation I assume?

pichlermarc

A few more comments, then this should be good to merge. 🙂
You can expect this to release within the next two weeks (which is the rough release-schedule we try to keep).

pichlermarc · 2024-01-18T09:49:54Z

.../packages/opentelemetry-exporter-metrics-otlp-http/test/node/CollectorMetricExporter.test.ts

+
+    it('aggregationSelector returns the default aggregation preference when nothing is supplied', () => {
+      const exporter = new OTLPMetricExporter({
+        temporalityPreference: AggregationTemporalityPreference.CUMULATIVE,


I think we can remove it since it does not affect the test in any way and we don't assert on it. 🙂

Suggested change

temporalityPreference: AggregationTemporalityPreference.CUMULATIVE,

experimental/packages/opentelemetry-exporter-metrics-otlp-http/src/OTLPMetricExporterBase.ts

pichlermarc · 2024-01-25T07:12:20Z

experimental/packages/opentelemetry-exporter-metrics-otlp-http/src/OTLPMetricExporterOptions.ts


 export interface OTLPMetricExporterOptions extends OTLPExporterConfigBase {
  temporalityPreference?:
    | AggregationTemporalityPreference
    | AggregationTemporality;
+  aggregationSelector?: AggregationSelector;


I think preference may be the better name as it can be overwritten by a view.

Suggested change

aggregationSelector?: AggregationSelector;

aggregationPreference?: AggregationSelector;

pichlermarc · 2024-01-25T07:16:03Z

experimental/CHANGELOG.md

@@ -8,6 +8,8 @@ All notable changes to experimental packages in this project will be documented

 ### :rocket: (Enhancement)

+* feat(exporter-metrics-otlp-http) [#4409](https://github.com/open-telemetry/opentelemetry-js/pull/4409) @AkselAllas


Suggested change

* feat(exporter-metrics-otlp-http) [#4409](https://github.com/open-telemetry/opentelemetry-js/pull/4409) @AkselAllas

* feat(exporter-metrics-otlp-http): add option to set the exporter aggregation preference [#4409](https://github.com/open-telemetry/opentelemetry-js/pull/4409) @AkselAllas

…rter

pichlermarc

Looks good, thanks for taking care of this 🙂

…elemetry#4409)

AkselAllas requested a review from a team January 10, 2024 15:18

pichlermarc reviewed Jan 12, 2024

View reviewed changes

AkselAllas force-pushed the add-aggregation-selector-to-otlp-metric-exporter branch from 4819ba2 to e0f076d Compare January 17, 2024 13:40

AkselAllas requested a review from pichlermarc January 17, 2024 13:43

AkselAllas commented Jan 17, 2024

View reviewed changes

experimental/packages/opentelemetry-exporter-metrics-otlp-http/src/OTLPMetricExporterBase.ts Show resolved Hide resolved

AkselAllas force-pushed the add-aggregation-selector-to-otlp-metric-exporter branch 2 times, most recently from 0fad7d0 to 396b6d1 Compare January 17, 2024 18:04

AkselAllas force-pushed the add-aggregation-selector-to-otlp-metric-exporter branch from 86df50a to b80e84d Compare January 18, 2024 10:53

pichlermarc changed the title ~~feat(add-aggregation-selector-option-to-otlp-metric-exporter)~~ feat(exporter-metrics-otlp-http): add option to set the exporter aggregation preference Jan 25, 2024

pichlermarc reviewed Jan 25, 2024

View reviewed changes

pichlermarc added pkg:exporter-metrics-otlp-grpc pkg:exporter-metrics-otlp-http pkg:exporter-metrics-otlp-proto labels Jan 25, 2024

feat(add-aggregation-selector-option-to-otlp-metric-exporter)

590392d

AkselAllas force-pushed the add-aggregation-selector-to-otlp-metric-exporter branch from a2246ed to 590392d Compare January 26, 2024 11:28

Merge branch 'main' into add-aggregation-selector-to-otlp-metric-expo…

6072a65

…rter

AkselAllas requested a review from pichlermarc January 26, 2024 11:29

pichlermarc approved these changes Jan 29, 2024

View reviewed changes

pichlermarc merged commit 4655895 into open-telemetry:main Jan 29, 2024
20 checks passed

pichlermarc mentioned this pull request Feb 23, 2024

chore: prepare release API 1.8.0/Core 1.21.0/Experimental 0.49.0 #4504

Merged

This was referenced May 19, 2024

[Snyk] Upgrade @opentelemetry/exporter-trace-otlp-http from 0.38.0 to 0.51.0 nicholaschoi168/opentelemetry.io#6

Open

[Snyk] Upgrade @opentelemetry/instrumentation from 0.38.0 to 0.51.0 nicholaschoi168/opentelemetry.io#7

Open

Graysonbarton mentioned this pull request May 21, 2024

[Snyk] Upgrade @opentelemetry/instrumentation from 0.43.0 to 0.51.0 Graysonbarton/synthetics-sdk-nodejs#5

Open

Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024

feat(add-aggregation-selector-option-to-otlp-metric-exporter) (open-t…

ebc552a

…elemetry#4409)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(exporter-metrics-otlp-http): add option to set the exporter aggregation preference #4409

feat(exporter-metrics-otlp-http): add option to set the exporter aggregation preference #4409

AkselAllas commented Jan 10, 2024

linux-foundation-easycla bot commented Jan 10, 2024 •

edited

Loading

AkselAllas commented Jan 12, 2024

pichlermarc left a comment

pichlermarc Jan 12, 2024

AkselAllas Jan 17, 2024

pichlermarc Jan 17, 2024

AkselAllas Jan 17, 2024 •

edited

Loading

AkselAllas commented Jan 17, 2024

codecov bot commented Jan 17, 2024 •

edited

Loading

trentm commented Jan 17, 2024

AkselAllas commented Jan 18, 2024

AkselAllas commented Jan 22, 2024 •

edited

Loading

pichlermarc left a comment

pichlermarc Jan 18, 2024

AkselAllas Jan 26, 2024

pichlermarc Jan 25, 2024

AkselAllas Jan 26, 2024

pichlermarc Jan 25, 2024

AkselAllas Jan 26, 2024

pichlermarc left a comment

	aggregationSelector?: AggregationSelector;
	aggregationPreference?: AggregationSelector;

		@@ -8,6 +8,8 @@ All notable changes to experimental packages in this project will be documented

		### :rocket: (Enhancement)

		* feat(exporter-metrics-otlp-http) [#4409](https://github.com/open-telemetry/opentelemetry-js/pull/4409) @AkselAllas

	* feat(exporter-metrics-otlp-http) [#4409](https://github.com/open-telemetry/opentelemetry-js/pull/4409) @AkselAllas
	* feat(exporter-metrics-otlp-http): add option to set the exporter aggregation preference [#4409](https://github.com/open-telemetry/opentelemetry-js/pull/4409) @AkselAllas

feat(exporter-metrics-otlp-http): add option to set the exporter aggregation preference #4409

feat(exporter-metrics-otlp-http): add option to set the exporter aggregation preference #4409

Conversation

AkselAllas commented Jan 10, 2024

linux-foundation-easycla bot commented Jan 10, 2024 • edited Loading

AkselAllas commented Jan 12, 2024

pichlermarc 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

AkselAllas Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

AkselAllas commented Jan 17, 2024

codecov bot commented Jan 17, 2024 • edited Loading

Codecov Report

trentm commented Jan 17, 2024

AkselAllas commented Jan 18, 2024

AkselAllas commented Jan 22, 2024 • edited Loading

pichlermarc 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pichlermarc left a comment

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Jan 10, 2024 •

edited

Loading

AkselAllas Jan 17, 2024 •

edited

Loading

codecov bot commented Jan 17, 2024 •

edited

Loading

AkselAllas commented Jan 22, 2024 •

edited

Loading