feat(experimental-packages): Update packages to latest SDK Version. #2871

pichlermarc · 2022-03-28T18:50:10Z

Which problem is this PR solving?

This PR

removes the -wip suffix from api-metrics and metrics-sdk-base, this causes lerna to link the local version of the packages. This prompts the following updates:
- Metrics Exporters to the latest Metrics SDK (exporter-metrics-otlp-grpc, exporter-metrics-otlp-http, exporter-metrics-otlp-proto)
- Update of opentelemetry-sdk-node to the latest Metrics SDK.
- Update of otlp-transformer to the latest Metrics SDK.
updates the dependencies to stable packages to 1.1.1 which required updates to:
- all instrumentation-* packages due to parseUrl from sdk-trace-web does not support relative paths anymore #2884

I apologize for this huge PR, it was discussed and it is, unfortunately, necessary. The *-wip packages pulled in 1.0.x, and leaving it out caused the build of this PR (originally only an update to the OTLP Metrics Exporters) to fail (see also: comments on #2874). This was due to their dependency on sdk-trace-base via exporter-trace-otlp-http.

Original Issue #2774
Supersedes #2874

Type of change

New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Updated unit tests

Checklist:

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

experimental/packages/opentelemetry-exporter-metrics-otlp-grpc/package.json

codecov · 2022-03-29T13:19:05Z

Codecov Report

Merging #2871 (54a6166) into main (eac583e) will decrease coverage by 1.19%.
The diff coverage is n/a.

❗ Current head 54a6166 differs from pull request most recent head 68f0fef. Consider uploading reports for the commit 68f0fef to get more accurate results

@@            Coverage Diff             @@
##             main    #2871      +/-   ##
==========================================
- Coverage   93.42%   92.22%   -1.20%     
==========================================
  Files         162       68      -94     
  Lines        5564     1993    -3571     
  Branches     1175      435     -740     
==========================================
- Hits         5198     1838    -3360     
+ Misses        366      155     -211

Impacted Files	Coverage Δ
...ckages/opentelemetry-sdk-trace-web/karma.worker.js	`0.00% <0.00%> (-100.00%)`	⬇️
...s/opentelemetry-core/src/platform/node/sdk-info.ts	`0.00% <0.00%> (-100.00%)`	⬇️
...opentelemetry-core/src/platform/node/globalThis.ts	`0.00% <0.00%> (-100.00%)`	⬇️
...pentelemetry-core/src/platform/node/performance.ts	`0.00% <0.00%> (-100.00%)`	⬇️
...emetry-api-metrics/src/platform/node/globalThis.ts	`0.00% <0.00%> (-100.00%)`	⬇️
...lemetry-resources/src/detectors/ProcessDetector.ts	`31.81% <0.00%> (-68.19%)`	⬇️
...ckages/opentelemetry-exporter-zipkin/src/zipkin.ts	`84.48% <0.00%> (-15.52%)`	⬇️
...lemetry-resources/src/detectors/BrowserDetector.ts	`93.33% <0.00%> (-6.67%)`	⬇️
...exporter-metrics-otlp-http/src/transformMetrics.ts	`91.37% <0.00%> (-4.28%)`	⬇️
...s/opentelemetry-instrumentation-fetch/src/fetch.ts	`96.53% <0.00%> (-0.48%)`	⬇️
... and 99 more

seemk

What about the proto transformer package (introduced in #2746)? Should it be used as some of the proto types in this PR are a bit out of date, i.e. intSum and doubleSum metricfields?

pichlermarc · 2022-03-31T09:05:09Z

What about the proto transformer package (introduced in #2746)? Should it be used as some of the proto types in this PR are a bit out of date, i.e. intSum and doubleSum metricfields?

True. Since we're now dropping the -wip from the package names we'll have to update that package this in this PR anyway. I'm working on that right now 🙂

rauno56 · 2022-03-31T13:55:07Z

We later discussed changes in this PR after the official part of the SIG meeting.

To my understanding, updating anything other than devDeps is the way we want to go.

devdependencies are mostly pinned and should be updated.
Ranges in dependencies and peerDependencies should reflect what's actually required and supported aiming for as broad of an range as possible to allow for maximum npm-deduping and user specification.

The problems(mentioned here) elsewhere were caused by an invalid bump in peerDependency range from >=1.0.0 <1.2.0 to >=1.1.0 <1.2.0.

If we merge this we force the whole userbase(that mainly uses caret-ranges for SDKs) update the API and in turn everything else and run into potential issues with implementing packages that do not support the newest API.

EDIT

Above actually only applies to peer dependency ranges, which, hopefully, only widen from the "top" in the context of one major API version. To conclude, my reconsidered stance is: don't change peer dependency ranges stricter, but updating everything else is fine.

Flarna · 2022-03-31T18:13:51Z

NPM doesn't dedup everything even ranges would allow it. I haven't fully understood this and it depends on the NPM version.
But I think newest NPM version install peer deps automatically and this install process seems to be not fully integrated in the dedup process.

I think the problem is not the API alone.

SDK implements API therefore minor API versions are (potentially) breaking for SDK
This is very similar for minor SDK versions which may result in breaking e.g. exporters/spanprocessors as they implement the SDK interface

Maybe it's better to create a dedicated issue to discuss and agree an a versioning schema. This PR is already complex enough to get it somewhat running. Tuning the devDeps/Deps ranges in a followup (before release) can be done).

rauno56 · 2022-04-01T07:17:48Z

Sure. Comments are welcome: #2863

pichlermarc · 2022-04-01T15:12:48Z

A quick update on the progress of this PR:
I am now trying to figure out how to get the browser tests for instrumentation-fetch to pass.
I will reset the peer dependencies to what is actually needed by the packages once I am done with this.

Sorry that this is taking so long, most of these packages are new to me.

seemk · 2022-04-05T12:31:24Z

Not related to your changes, but noticed that exporting of createExportMetricsServiceRequest and the metric types are commented out in the transformer package. 👀

pichlermarc · 2022-04-05T13:30:58Z

Not related to your changes, but noticed that exporting of createExportMetricsServiceRequest and the metric types are commented out in the transformer package. eyes

Yes, I noticed that too. I just talked to @dyladan and am working on a follow-up PR already. To make it easier to change the exporter packages to use the otlp-transformer package individually, we will introduce a otlp-exporter-base package first. This way the metrics exporters do not need to depend on the trace exporters anymore. Then we will update the exporters individually to make reviews easier. 🙂

We could continue the discussion on this in #2774 🙂

dyladan · 2022-04-05T13:43:10Z

Not related to your changes, but noticed that exporting of createExportMetricsServiceRequest and the metric types are commented out in the transformer package. 👀

Yes this was done because the SDK types were still WIP at that time and I wanted to get the package merged with the idea that it could be updated in the future. Now that those types are settled it can be updated and the exports can be added back.

dyladan · 2022-04-06T14:48:30Z

CHANGELOG.md

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

 ### :rocket: (Enhancement)

+* [#2871](https://github.com/open-telemetry/opentelemetry-js/pull/2871) feat(experimental-packages): Update packages to latest SDK Version. ([@pichlermarc](https://github.com/pichlermarc))


Can you add sub-bullet points to this? This PR handles a lot more work than what is implied by this title.

We can shorten this entry to be like:

Suggested change

* [#2871](https://github.com/open-telemetry/opentelemetry-js/pull/2871) feat(experimental-packages): Update packages to latest SDK Version. ([@pichlermarc](https://github.com/pichlermarc))

* feat(experimental-packages): Update packages to latest SDK Version. #2871 @pichlermarc

@dyladan Good idea, I added sub-bullet points. in ecc38bc.

I've submitted #2889 to discuss the verbose format.

@legendecas yes, this seems way more concise, good idea. 🙂 I updated the entry to #2889 format in ab80dba, and moved the entry to the experimental changelog. However, this format does not work for anyone viewing the changelog file locally.

experimental/packages/opentelemetry-sdk-node/src/types.ts

...ckages/opentelemetry-exporter-metrics-otlp-http/test/browser/CollectorMetricExporter.test.ts

legendecas

Overall LGTM

dyladan · 2022-04-06T20:34:39Z

@open-telemetry/javascript-maintainers I'm going to merge this tomorrow if there are no objections

experimental/packages/exporter-trace-otlp-grpc/package.json

… move entry to experimental changelog.

Ugzuzg · 2022-05-26T18:57:52Z

experimental/packages/opentelemetry-instrumentation-fetch/src/fetch.ts

@@ -316,16 +321,24 @@ export class FetchInstrumentation extends InstrumentationBase<

        function endSpanOnSuccess(span: api.Span, response: Response) {
          plugin._applyAttributesAfterFetch(span, options, response);
+          const spanResponse = {


Was that meant to be used as a third argument of _endSpan call? Right now the spanResponse variable is unused.

Sorry, I did not see your comment on this PR. Looks like this is some leftover from me trying to figure out what eventually turned out to be caused by #2884. I unintentionally added this to commit f6d0b0f.

pichlermarc changed the title ~~feature(otlp-metrics-exporter): update OTLP metrics exporters to use latest SDK~~ feat(otlp-metrics-exporter): update OTLP metrics exporters to use latest SDK Mar 28, 2022

Flarna reviewed Mar 28, 2022

View reviewed changes

experimental/packages/opentelemetry-exporter-metrics-otlp-grpc/package.json Outdated Show resolved Hide resolved

legendecas mentioned this pull request Mar 29, 2022

feat: remove -wip suffix in metrics packages #2872

Closed

2 tasks

pichlermarc force-pushed the update-otlp-metric-exporter branch from e373187 to f1ba2fb Compare March 29, 2022 18:55

seemk reviewed Mar 30, 2022

View reviewed changes

pichlermarc force-pushed the update-otlp-metric-exporter branch from 64b8e84 to 1f9a214 Compare March 30, 2022 15:21

pichlermarc mentioned this pull request Mar 31, 2022

chore: update OpenTelemetry deps in all experimental packages #2874

Closed

pichlermarc force-pushed the update-otlp-metric-exporter branch from 1f9a214 to ff547ff Compare March 31, 2022 12:41

pichlermarc force-pushed the update-otlp-metric-exporter branch from 1c294db to da6e9b3 Compare April 1, 2022 12:44

pichlermarc force-pushed the update-otlp-metric-exporter branch from a3078b6 to df55205 Compare April 4, 2022 12:52

pichlermarc changed the title ~~feat(otlp-metrics-exporter): update OTLP metrics exporters to use latest SDK~~ feat(experimental-packages): Update packages to latest SDK Version. Apr 4, 2022

pichlermarc mentioned this pull request Apr 4, 2022

parseUrl from sdk-trace-web does not support relative paths anymore #2884

Closed

pichlermarc added 11 commits April 4, 2022 18:28

feature(otlp-metrics-exporter): begin exporter update.

08208e3

feature(exporter): continue exporter update.

1c66ce2

feat(otlp-http-exporter): update common tests.

85a8b53

feat(otlp-http-exporter): update node tests.

a5cb790

feat(otlp-grpc-exporter): prelimiary test update.

59c146c

feat(otlp-grpc-exporter): update tests.

780c77d

chore: updating submodule for opentelemetry-proto

99b001b

chore: updating submodule for opentelemetry-proto

91b1289

feat(otlp-proto-exporter): update tests.

b27ce50

feat(otlp-http-exporter): update browser tests.

f2f4449

feat(otlp-http-exporter): update tranformation tests.

fc09a82

pichlermarc added 2 commits April 5, 2022 13:39

fix(otlp-transformer): remove unnecessary use of Array.from()

a192be4

fix(otlp-transformer): use else if in singular data point conversion.

dea3327

pichlermarc mentioned this pull request Apr 5, 2022

Update proto versions of OTLP exporters #2886

Closed

3 tasks

dyladan approved these changes Apr 6, 2022

View reviewed changes

legendecas reviewed Apr 6, 2022

View reviewed changes

experimental/packages/opentelemetry-sdk-node/src/types.ts Outdated Show resolved Hide resolved

...ckages/opentelemetry-exporter-metrics-otlp-http/test/browser/CollectorMetricExporter.test.ts Outdated Show resolved Hide resolved

fix(changelog): update changelog to include a longer PR description.

ecc38bc

legendecas approved these changes Apr 6, 2022

View reviewed changes

pichlermarc added 3 commits April 6, 2022 22:01

fix(metrics-exporter): fix test grouping.

cc1af71

Merge branch 'main' into update-otlp-metric-exporter

e1a5f88

fix(sdk-node): remove metricInterval.

e376768

rauno56 reviewed Apr 7, 2022

View reviewed changes

experimental/packages/exporter-trace-otlp-grpc/package.json Show resolved Hide resolved

pichlermarc added 2 commits April 7, 2022 09:42

fix(metrics-exporter): use index accessor for private properties.

fcffd62

fix(changelog): update changelog entry to open-telemetry#2889 format,…

ab80dba

… move entry to experimental changelog.

vmarchaud approved these changes Apr 7, 2022

View reviewed changes

rauno56 approved these changes Apr 7, 2022

View reviewed changes

Merge branch 'main' into update-otlp-metric-exporter

68f0fef

dyladan merged commit 0213d82 into open-telemetry:main Apr 7, 2022

dyladan deleted the update-otlp-metric-exporter branch April 7, 2022 20:51

legendecas mentioned this pull request Apr 8, 2022

chore: update core dependcencies in experimental #2870

Closed

pichlermarc mentioned this pull request Apr 8, 2022

Flaky browser test #2852

Closed

pichlermarc mentioned this pull request Apr 22, 2022

REQUEST: New membership for @pichlermarc open-telemetry/community#1039

Closed

6 tasks

Ugzuzg reviewed May 26, 2022

View reviewed changes

dyladan mentioned this pull request Jun 17, 2022

refactor(fetch): revert to original code in endSpanOnSuccess #3037

Merged

6 tasks

pichlermarc mentioned this pull request Sep 27, 2022

Refactor OTLP Metrics Exporters #3276

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(experimental-packages): Update packages to latest SDK Version. #2871

feat(experimental-packages): Update packages to latest SDK Version. #2871

pichlermarc commented Mar 28, 2022 •

edited

Loading

codecov bot commented Mar 29, 2022 •

edited

Loading

seemk left a comment •

edited

Loading

pichlermarc commented Mar 31, 2022

rauno56 commented Mar 31, 2022 •

edited

Loading

Flarna commented Mar 31, 2022

rauno56 commented Apr 1, 2022

pichlermarc commented Apr 1, 2022

seemk commented Apr 5, 2022

pichlermarc commented Apr 5, 2022

dyladan commented Apr 5, 2022

dyladan Apr 6, 2022

legendecas Apr 6, 2022

pichlermarc Apr 6, 2022

legendecas Apr 6, 2022

pichlermarc Apr 7, 2022

legendecas left a comment

dyladan commented Apr 6, 2022

Ugzuzg May 26, 2022

pichlermarc Jun 20, 2022

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

		### :rocket: (Enhancement)

		* [#2871](https://github.com/open-telemetry/opentelemetry-js/pull/2871) feat(experimental-packages): Update packages to latest SDK Version. ([@pichlermarc](https://github.com/pichlermarc))

	* [#2871](https://github.com/open-telemetry/opentelemetry-js/pull/2871) feat(experimental-packages): Update packages to latest SDK Version. ([@pichlermarc](https://github.com/pichlermarc))
	* feat(experimental-packages): Update packages to latest SDK Version. #2871 @pichlermarc

feat(experimental-packages): Update packages to latest SDK Version. #2871

feat(experimental-packages): Update packages to latest SDK Version. #2871

Conversation

pichlermarc commented Mar 28, 2022 • edited Loading

Which problem is this PR solving?

Type of change

How Has This Been Tested?

Checklist:

codecov bot commented Mar 29, 2022 • edited Loading

Codecov Report

seemk left a comment • edited Loading

Choose a reason for hiding this comment

pichlermarc commented Mar 31, 2022

rauno56 commented Mar 31, 2022 • edited Loading

Flarna commented Mar 31, 2022

rauno56 commented Apr 1, 2022

pichlermarc commented Apr 1, 2022

seemk commented Apr 5, 2022

pichlermarc commented Apr 5, 2022

dyladan commented Apr 5, 2022

dyladan Apr 6, 2022

Choose a reason for hiding this comment

legendecas Apr 6, 2022

Choose a reason for hiding this comment

pichlermarc Apr 6, 2022

Choose a reason for hiding this comment

legendecas Apr 6, 2022

Choose a reason for hiding this comment

pichlermarc Apr 7, 2022

Choose a reason for hiding this comment

legendecas left a comment

Choose a reason for hiding this comment

dyladan commented Apr 6, 2022

Ugzuzg May 26, 2022

Choose a reason for hiding this comment

pichlermarc Jun 20, 2022

Choose a reason for hiding this comment

pichlermarc commented Mar 28, 2022 •

edited

Loading

codecov bot commented Mar 29, 2022 •

edited

Loading

seemk left a comment •

edited

Loading

rauno56 commented Mar 31, 2022 •

edited

Loading