feat!: Split metric and trace exporters into new experimental packages #2485

willarmiros · 2021-09-22T09:35:29Z

Which problem is this PR solving?

Fixes Remove metrics exporters before declaring OTLP exporters stable #2486

Background

This PR removes all metric logic from the main OTLP exporter packages and moves them into new packages. I am leaving this PR in draft until the updated core exporters are released.

Short description of the changes

Introduced 3 new experimental packages:
- @opentelemetry/exporter-metrics-otlp-http
- @opentelemetry/exporter-metrics-otlp-grpc
- @opentelemetry/exporter-metrics-otlp-proto
Copied the original package’s non-source code files to “bootstrap” the new packages
Separated (most) signal-specific sample code/information into the new READMEs to avoid replication
Then separated metric business logic into 3 new packages in experimental directory
Left all util methods in the trace exporters
Had to export more methods from the trace exporter packages, including the abstract base classes and util methods, so that metric exporters could depend on them. This could lead to a bigger surface area for breaking changes
Needed to export the OTLPExporterBrowserBase class from the browser class - just want to make sure this is ok since I couldn't find any other classes exported from a browser platform

Remaining blockers

Figure out the 2 failing export unit tests for metrics-otlp-grpc. @obecny I don't think the tests will be reproduced here but maybe you can help me take a look.
Sanity check test this using a real collector

codecov · 2021-09-22T09:38:30Z

Codecov Report

Merging #2485 (7ecf82d) into main (4e311bd) will increase coverage by 2.67%.
The diff coverage is n/a.

❗ Current head 7ecf82d differs from pull request most recent head e068039. Consider uploading reports for the commit e068039 to get more accurate results

@@            Coverage Diff             @@
##             main    #2485      +/-   ##
==========================================
+ Coverage   93.07%   95.74%   +2.67%     
==========================================
  Files         140       10     -130     
  Lines        5169      541    -4628     
  Branches     1101       99    -1002     
==========================================
- Hits         4811      518    -4293     
+ Misses        358       23     -335

Impacted Files	Coverage Δ
packages/opentelemetry-core/src/utils/url.ts
...ackages/opentelemetry-api-metrics/src/NoopMeter.ts
...y-instrumentation-http/src/enums/AttributeNames.ts
...elemetry-sdk-trace-base/src/BasicTracerProvider.ts
...ry-instrumentation-grpc/src/grpc-js/serverUtils.ts
...ckages/opentelemetry-core/src/utils/environment.ts
...ckages/opentelemetry-sdk-metrics-base/src/Utils.ts
packages/opentelemetry-core/src/utils/wrap.ts
...elemetry-exporter-zipkin/src/platform/node/util.ts
...emetry-instrumentation-grpc/src/instrumentation.ts
... and 122 more

willarmiros · 2021-09-22T10:20:54Z

@obecny I'm not sure if this is meaningful, but I saw you wrote (or at least refactored) these exporters. The Metrics gRPC exporter is failing two of the export unit tests after I moved it to experimental - one with TLS, and another without TLS but with metadata. The strange thing is they were (obviously) passing beforehand as part of the stable package, and the gRPC Trace exporter tests, which are nearly identical, still pass no problem.

The closest thing to a reason I've identified for the failure is here in exporter-otlp-grpc. The trace export tests that pass seem to call this send method twice, storing the request in the grpcQueue the first time and actually sending it the second time. The metrics TLS test that fails only calls send once, and never sends the request. I just have no clue why it only calls send once, and I also may be totally barking up the wrong tree.

Anyway, here's the error output, please let me know if there's anything obvious:

npm run test

> @opentelemetry/exporter-metrics-otlp-grpc@0.25.0 test /Users/armiros/opentelemetry/js/opentelemetry-js/willarmiros/opentelemetry-js/experimental/packages/opentelemetry-exporter-metrics-otlp-grpc
> nyc ts-mocha -p tsconfig.json 'test/**/*.test.ts'



  OTLPMetricExporter - node (getDefaultUrl)
    ✓ should default to localhost
    ✓ should keep the URL if included

  when configuring via environment
    ✓ should use url defined in env
    ✓ should override global exporter url with signal url defined in env
    ✓ should use headers defined via env
    ✓ should override global headers config with signal headers defined via env

  OTLPMetricExporter - node with TLS, without metadata
    instance
      ✓ should warn about headers
      ✓ should warn about path in url
    export
      1) should export metrics

  OTLPMetricExporter - node without TLS, without metadata
    instance
      ✓ should warn about headers
      ✓ should warn about path in url
    export
      ✓ should export metrics (504ms)

  OTLPMetricExporter - node without TLS, with metadata
    instance
      ✓ should warn about headers
      ✓ should warn about path in url
    export
      2) should export metrics


  13 passing (2s)
  2 failing

  1) OTLPMetricExporter - node with TLS, without metadata
       export
         should export metrics:

      Uncaught AssertionError [ERR_ASSERTION]: resource doesn't exist
      + expected - actual

      -false
      +true
      
      at Timeout._onTimeout (test/OTLPMetricExporter.test.ts:197:18)
      at listOnTimeout (internal/timers.js:551:17)
      at processTimers (internal/timers.js:494:7)

  2) OTLPMetricExporter - node without TLS, with metadata
       export
         should export metrics:

      Uncaught AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected

+ {}
- {
-   k: 'v'
- }
      + expected - actual

      -{}
      +{
      +  "k": "v"
      +}
      
      at Object.ensureMetadataIsCorrect (test/metricsHelper.ts:250:10)
      at Timeout._onTimeout (test/OTLPMetricExporter.test.ts:233:13)
      at listOnTimeout (internal/timers.js:551:17)
      at processTimers (internal/timers.js:494:7)

willarmiros · 2021-09-22T10:29:52Z

Also, the tests are failing because the experimental metric packages depend on the stable packages. However because of the separation between experimental/stable, the experimental packages try to pull in the newly-renamed trace exporter packages and fail to find them.

However with the exception of the above failing tests, I have verified the rest of the refactor by publishing the stable exporters to a local registry (verdaccio) and using that for experimental package testing.

willarmiros · 2021-09-27T15:35:39Z

Blocked on #2490, then will rebase and open for review.

vmarchaud

lgtm, i would wait for @obecny review since he wrote all the otlp exporter.

vmarchaud · 2021-10-06T07:55:45Z

Wouldn't it make sense to rename the current otlp-exporter to trace-otlp-exporter when releasing it as 1.0 though ?

willarmiros · 2021-10-06T21:35:42Z

@vmarchaud I think that'd make sense, but I'd be hesitant for a few reasons:

When metrics are stable, we might re-merge their exporters to be alongside trace exporters, which would again require a generic name
We just renamed the exporters in the last release, and I worry about the customer experience of having a package renamed twice in back-to-back releases

Of course would like to hear from the other maintainers as well though

vmarchaud · 2021-10-07T07:50:44Z

About 1), i think it make more sense of having different exporters for each signal, since they are generally packaged by distro i don't really see the point of trying to have them in a single package, WDYT ?

For 2) You are right about that but i would prefer to make that change now instead of after 1.0 them

willarmiros · 2021-10-07T22:15:41Z

Fair enough, I'm not against renaming to include trace in the name, given that it'll be the final renaming. If @dyladan and @obecny agree I can do the rename in this PR.

rauno56

Wouldn't it make sense to rename the current otlp-exporter to trace-otlp-exporter when releasing it as 1.0 though ?

IMO, Whether it makes sense or not depends on if we intend to add metrics functionality into otlp-exporter or not once metric-specific functionality is stable.
If we do, let's not rename those packages back-and-forth to reflect their current state, but just think of otlp-exporter as "the general purpose exporter which just happens to only have tracing support right now, but eventually will have more".

That said, I wouldn't rename otlp-exporter to trace-otlp-exporter.

willarmiros · 2021-10-26T06:38:10Z

@dyladan just to confirm before I make this last change, after this PR is merged there will be 6 new packages in experimental:

@opentelemetry/exporter-metrics-otlp-http
@opentelemetry/exporter-metrics-otlp-grpc
@opentelemetry/exporter-metrics-otlp-proto
@opentelemetry/exporter-trace-otlp-http
@opentelemetry/exporter-trace-otlp-grpc
@opentelemetry/exporter-trace-otlp-proto

Does that look right?

Also, should I go ahead and rename the directories of the trace exporters to include trace? E.g.

packages/opentelemetry-exporter-otlp-grpc -> packages/opentelemetry-exporter-trace-otlp-grpc

dyladan · 2021-10-26T13:06:22Z

@dyladan just to confirm before I make this last change, after this PR is merged there will be 6 new packages in experimental:

@opentelemetry/exporter-metrics-otlp-http

@opentelemetry/exporter-metrics-otlp-grpc

@opentelemetry/exporter-metrics-otlp-proto

@opentelemetry/exporter-trace-otlp-http

@opentelemetry/exporter-trace-otlp-grpc

@opentelemetry/exporter-trace-otlp-proto

Does that look right?

Also, should I go ahead and rename the directories of the trace exporters to include trace? E.g.
packages/opentelemetry-exporter-otlp-grpc -> packages/opentelemetry-exporter-trace-otlp-grpc

Yes to both

willarmiros · 2021-10-26T17:41:54Z

@open-telemetry/javascript-approvers this should be ready for review now, PTAL!

dyladan · 2021-10-26T20:38:59Z

This PR is absolutely enormous which makes it difficult to review. Can you confirm some things for me?

All tests were retained
Aside from the changes required to actually set up the packages (tsconfig, lint, etc) the only source changes were file moves and moving whole classes. No actual implementations or functionalities were changed.

willarmiros · 2021-10-26T20:45:16Z

This PR is absolutely enormous which makes it difficult to review. Can you confirm some things for me?

All tests were retained
Aside from the changes required to actually set up the packages (tsconfig, lint, etc) the only source changes were file moves and moving whole classes. No actual implementations or functionalities were changed.

Yes. The only source code files I manipulated in non-automated ways were the traceHelper.ts and metricHelper.ts files used for tests. Originally there was just one helper.ts for each exporter that included a bunch of helper functions for both trace and metric tests. So to separate them I had to manually copy trace helper functions into one file & metrics helpers into another.

…telemetry-js into split-exporters

willarmiros · 2021-10-28T00:53:24Z

@dyladan @vmarchaud hopefully the end of these pesky conflicts 😄

vmarchaud · 2021-10-28T08:10:12Z

@obecny I would prefer to have your approval since you initialy wrote all otlp exporter

obecny · 2021-10-28T10:56:55Z

@obecny I would prefer to have your approval since you initially wrote all otlp exporter

I raised couple times that we could do few things differently and have separate class for converting data - something that @blumamir was trying to achieve some time ago.

I don't want to block it but also I had quite different view of how it could be done. My confidence level for so many changes is really low and it is impossible for me to be sure that nothing is broken. I wanted to leave things in experimental as they are and then create new things (including refactoring) out of this so we will have longer time for people to change to new packages and still have a backup plan if anything doesn't work as expected. We would be having "doubled" packages for some time but at least we will not break anything without giving people a chance to compare both packages - only downgrade will be possible :/. I simply don't feel confident enough with so many changes and being already "stable".

dyladan · 2021-10-28T12:07:17Z

Nothing here is stable. These packages are being created in the experimental folder. The reason for the extensive test suites is to be able to do large refactorings and have some level of confidence that the package still works. These will still live in experimental for a period of time until we are sure nothing is broken. There is also nothing preventing it from being refactored with a new implementation.

willarmiros · 2021-10-29T17:29:06Z

@obecny I completely understand the hesitance for such a large change, the PR became bigger than I was hoping it would have to be. That's why I was very careful to make all code changes in automated ways (e.g. find and replace) except to the test helpers I mentioned earlier. We could test these changes by updating the examples to use the new exporter packages in a separate PR to gain more confidence from real apps.

I'm not sure how much value there would have been in still publishing the old joint exporter packages, since there presumably wouldn't be any changes made to it, so it would be the same as just using the current published version.

obecny · 2021-10-29T18:03:08Z

We could test these changes by updating the examples to use the new exporter packages in a separate PR to gain more confidence from real apps.

Hmm I think that should be checked before this PR is merged. The update of the examples can be separate PR, but I wish you make sure the new packages works fine with few examples before it will be merged (using lerna to bootstrap it). WDYT, can you run those tests ?

willarmiros · 2021-10-31T23:57:16Z

@obecny I validated that the OTLP exporter sample app works as expected for exporting both traces and metrics after these changes. You can see the changes in my other PR for the sample app: https://github.com/open-telemetry/opentelemetry-js/pull/2577/files#diff-ad989b2d87fe5747576d5a98f3819e195a3053857fd4b0de12c9f5417e0dadd4

dyladan · 2021-11-01T20:54:30Z

@obecny does that satisfy your concern? If so, I would merge this.

obecny · 2021-11-02T17:15:42Z

@obecny I validated that the OTLP exporter sample app works as expected for exporting both traces and metrics after these changes. You can see the changes in my other PR for the sample app: https://github.com/open-telemetry/opentelemetry-js/pull/2577/files#diff-ad989b2d87fe5747576d5a98f3819e195a3053857fd4b0de12c9f5417e0dadd4

thx, appreciate it !

William Armiros added 9 commits September 20, 2021 20:45

chore: updating submodule for opentelemetry-proto

32d7962

chore: initialized exporter-metrics-otlp-grpc package

f9972c8

feat\!: populated exporter-metrics-otlp-grpc package

9685772

chore: bootstrapped exporter-metrics-otlp-http package

007970c

feat\!: migrated metric exporter logic to exporter-metrics-otlp-http

9618274

chore: updating submodule for opentelemetry-proto

d2ecae0

chore: bootstrapped exporter-metrics-otlp-proto package

07b43bf

feat!: migrated proto logic to exporter-metrics-otlp-proto package

5ac7d7b

fix: fixed some issues in grpc and proto

308cfe3

William Armiros added 2 commits September 22, 2021 02:49

removed debug logging

16e7a95

added api to devdeps

c50cada

William Armiros added 2 commits September 28, 2021 20:30

merge branch 'main' into split-exporters

e5ed763

fixed imports

0751815

willarmiros marked this pull request as ready for review September 29, 2021 03:11

willarmiros requested a review from a team September 29, 2021 03:11

William Armiros added 2 commits October 5, 2021 16:23

merge 'main' into split-exporters

b886cb9

updated versions to latest

211fa12

vmarchaud approved these changes Oct 6, 2021

View reviewed changes

vmarchaud requested review from obecny and dyladan October 9, 2021 13:46

vmarchaud added the breaking label Oct 9, 2021

rauno56 approved these changes Oct 14, 2021

View reviewed changes

William Armiros added 3 commits October 26, 2021 10:09

renamed exporter-otlp-http -> exporter-trace-otlp-http

752463e

renamed exporter-otlp-grpc -> exporter-trace-otlp-grpc

c4aa74a

renamed exporter-otlp-proto -> exporter-trace-otlp-proto

3e7daf4

willarmiros changed the title ~~feat!: Split metric exporters into new experimental packages~~ feat!: Split metric and trace exporters into new experimental packages Oct 26, 2021

Merge branch 'main' into split-exporters

44de3a7

dyladan approved these changes Oct 26, 2021

View reviewed changes

vmarchaud and others added 3 commits October 27, 2021 09:44

Merge branch 'main' into split-exporters

8eccf90

fix conflicts and bump api version

2992e5c

Merge branch 'split-exporters' of https://github.com/willarmiros/open…

81891ec

…telemetry-js into split-exporters

willarmiros mentioned this pull request Oct 31, 2021

chore: updated exporter sample app to use new exporters #2577

Closed

Merge branch 'main' into split-exporters

353d038

Merge branch 'main' into split-exporters

e068039

obecny approved these changes Nov 2, 2021

View reviewed changes

dyladan merged commit c307949 into open-telemetry:main Nov 2, 2021

dyladan mentioned this pull request Nov 10, 2021

chore: release proposal v1.0.1 / v0.27.0 #2611

Merged

NathanielRN mentioned this pull request Nov 30, 2021

Node.js Layer using Depreated Exporter open-telemetry/opentelemetry-lambda#183

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Split metric and trace exporters into new experimental packages #2485

feat!: Split metric and trace exporters into new experimental packages #2485

willarmiros commented Sep 22, 2021 •

edited

Loading

codecov bot commented Sep 22, 2021 •

edited

Loading

willarmiros commented Sep 22, 2021

willarmiros commented Sep 22, 2021

willarmiros commented Sep 27, 2021

vmarchaud left a comment

vmarchaud commented Oct 6, 2021

willarmiros commented Oct 6, 2021

vmarchaud commented Oct 7, 2021 •

edited

Loading

willarmiros commented Oct 7, 2021

rauno56 left a comment •

edited

Loading

willarmiros commented Oct 26, 2021

dyladan commented Oct 26, 2021

willarmiros commented Oct 26, 2021

dyladan commented Oct 26, 2021

willarmiros commented Oct 26, 2021

willarmiros commented Oct 28, 2021

vmarchaud commented Oct 28, 2021

obecny commented Oct 28, 2021

dyladan commented Oct 28, 2021

willarmiros commented Oct 29, 2021

obecny commented Oct 29, 2021

willarmiros commented Oct 31, 2021

dyladan commented Nov 1, 2021

obecny commented Nov 2, 2021

feat!: Split metric and trace exporters into new experimental packages #2485

feat!: Split metric and trace exporters into new experimental packages #2485

Conversation

willarmiros commented Sep 22, 2021 • edited Loading

Which problem is this PR solving?

Background

Short description of the changes

Remaining blockers

codecov bot commented Sep 22, 2021 • edited Loading

Codecov Report

willarmiros commented Sep 22, 2021

willarmiros commented Sep 22, 2021

willarmiros commented Sep 27, 2021

vmarchaud left a comment

Choose a reason for hiding this comment

vmarchaud commented Oct 6, 2021

willarmiros commented Oct 6, 2021

vmarchaud commented Oct 7, 2021 • edited Loading

willarmiros commented Oct 7, 2021

rauno56 left a comment • edited Loading

Choose a reason for hiding this comment

willarmiros commented Oct 26, 2021

dyladan commented Oct 26, 2021

willarmiros commented Oct 26, 2021

dyladan commented Oct 26, 2021

willarmiros commented Oct 26, 2021

willarmiros commented Oct 28, 2021

vmarchaud commented Oct 28, 2021

obecny commented Oct 28, 2021

dyladan commented Oct 28, 2021

willarmiros commented Oct 29, 2021

obecny commented Oct 29, 2021

willarmiros commented Oct 31, 2021

dyladan commented Nov 1, 2021

obecny commented Nov 2, 2021

willarmiros commented Sep 22, 2021 •

edited

Loading

codecov bot commented Sep 22, 2021 •

edited

Loading

vmarchaud commented Oct 7, 2021 •

edited

Loading

rauno56 left a comment •

edited

Loading