feat(proto): add @opentelemetry/otlp-transformer package with hand-rolled transformation #2746

dyladan · 2022-01-28T20:49:33Z

related: #2665

This is an alternative to #2691 which includes only the types and hand-rolled transformation functions to convert traces and metrics into otlp json. Skipping protobufjs drastically reduces the complexity and size of the final binary, which will be important in the browser context.

It also includes a descriptor.json file in the root of the package which can be loaded by protobufjs and used to serialize messages into protobuf.

todo:

add an option to export traces in a protobuf-compatible way (the JSON representation requires trace and span IDs to be in hex, instead of the base64 format expected by the library)
update README to reflect the changes

codecov · 2022-01-28T20:53:25Z

Codecov Report

Merging #2746 (b6b7c40) into main (b0f8a2d) will not change coverage.
The diff coverage is n/a.

❗ Current head b6b7c40 differs from pull request most recent head 5be5d87. Consider uploading reports for the commit 5be5d87 to get more accurate results

@@           Coverage Diff           @@
##             main    #2746   +/-   ##
=======================================
  Coverage   93.19%   93.19%           
=======================================
  Files         157      157           
  Lines        5117     5117           
  Branches     1097     1097           
=======================================
  Hits         4769     4769           
  Misses        348      348

legendecas

LGTM % nits.

experimental/tsconfig.json

experimental/packages/proto/src/metrics/types.ts

experimental/packages/proto/src/common/internal.ts

morigs · 2022-02-21T13:13:11Z

Hi, sorry for disturbing, are there any plans to merge this in near future? I am concerned about #2675

dyladan · 2022-02-22T15:20:39Z

Hi, sorry for disturbing, are there any plans to merge this in near future? I am concerned about #2675

This currently on hold waiting on #2775

I will update the proto in the existing implementations to get #2675 unblocked so we don't need to rush this one.

dyladan · 2022-02-23T16:04:59Z

Converting to draft while we wait on #2775

dyladan · 2022-02-25T16:19:13Z

I think we have 2 final questions to resolve before merging this @open-telemetry/javascript-maintainers @open-telemetry/javascript-approvers

What should this package be named?

Right now I have it as @opentelemetry/proto but it actually doesn't return protobuf, only an object which uses the OTLP types. Maybe @opentelemetry/otlp-transformer? I'm open to suggestions. I don't expect this package to be used directly by users so a short friendly name isn't probably important.

How can it be stabilized?

The return type of each serialization function depends on the field names in the proto which are unstable. For example, createExportTraceServiceRequest returns IExportTraceServiceRequest which uses the name instrumentationLibrary. That name is currently being considered to be changed to InstrumentationScope in open-telemetry/opentelemetry-proto#362 (review)

dyladan · 2022-02-25T17:21:01Z

Marking as ready for review again as it is higher priority due to #2804

vmarchaud · 2022-02-25T20:22:18Z

What should this package be named?

I agree that it shouldn't refer to proto directly and more to the protocol itself so i'm fine with @opentelemetry/otlp-transformer, maybe use otlp-json-encoder since it only encode to json for now ?

How can it be stabilized?

I'm not really sure but could we just have optional option that specify which proto version we encode to ? Once it get merged we could add a if that changes the generated representation ?

dyladan · 2022-02-28T14:24:02Z

What should this package be named?

I agree that it shouldn't refer to proto directly and more to the protocol itself so i'm fine with @opentelemetry/otlp-transformer, maybe use otlp-json-encoder since it only encode to json for now ?

How can it be stabilized?

I'm not really sure but could we just have optional option that specify which proto version we encode to ? Once it get merged we could add a if that changes the generated representation ?

I would prefer not to do that if possible. I don't want to commit to maintaining all proto versions in parallel forever.

legendecas · 2022-02-28T16:39:45Z

How can it be stabilized?

It sounds to me that the problem here is similar to the JSON representation of the OTLP protocol since the names of the fields/types have to be part of the stabilized API. Are there any roadmaps on the stabilization of JSON representation?

What should this package be named?

@opentelemetry/otlp-transformer or just @opentelemetry/otlp? Both sound cool to me.

Marking as ready for review again as it is higher priority due to #2804

Did you forget to hit the button? The PR is still a draft.

legendecas · 2022-03-01T05:39:04Z

CI failure could be fixed by #2768

vmarchaud · 2022-03-01T10:32:54Z

I would prefer not to do that if possible. I don't want to commit to maintaining all proto versions in parallel forever.

I don't personaly think another way to do this expect waiting for stabilization :/

experimental/packages/otlp-transformer/descriptor.json

dyladan · 2022-03-01T13:20:42Z

I was thinking we could document in the tsdocs explicitly that we are generating protos of a certain version and that future minor versions may generate another version.

dyladan · 2022-03-01T14:00:47Z

We could also independently version the package because we know it will need to be versioned each time something like this happens

legendecas · 2022-03-01T15:33:27Z

We could also independently version the package because we know it will need to be versioned each time something like this happens

This sounds very similar to the experimental packages in the repo to me.

dyladan · 2022-03-01T20:10:53Z

This sounds very similar to the experimental packages in the repo to me.

In the past we've had a requirement that stable packages don't depend on experimental packages, but maybe we can make a documented exception here in order to avoid leaking the implementation details of the OTLP transformation?

One other option would be to make this package a private package and bundle it as a bundled dependency in the OTLP exporters https://docs.npmjs.com/cli/v8/configuring-npm/package-json#bundleddependencies. This might be a way to share this code without deploying it to NPM.

experimental/packages/otlp-transformer/src/common/internal.ts

legendecas

Overall LGTM, great work!

experimental/packages/otlp-transformer/src/metrics/index.ts

experimental/packages/otlp-transformer/src/trace/index.ts

…json transformation

dyladan · 2022-03-15T09:07:13Z

Sorry for force push i had the wrong email in the history

experimental/packages/otlp-transformer/src/common/internal.ts

experimental/packages/otlp-transformer/src/common/types.ts

experimental/packages/otlp-transformer/src/trace/internal.ts

seemk

LGTM! 👍

seemk · 2022-03-21T20:43:43Z

experimental/packages/otlp-transformer/src/metrics/index.ts

+      const instrumentationLibraryMetrics = ilmEntry.value;
+      if (instrumentationLibraryMetrics.length > 0) {
+        const lib = instrumentationLibraryMetrics[0].instrumentationLibrary;
+        resourceMetrics.push({ instrumentationLibrary: lib, instrumentationLibraryMetrics, schemaUrl: lib.schemaUrl });


Noticed 1 thing - the instrumentation library schema URL is converted to the resource schema URL. Is it supposed to be like this? See the comment here: https://github.com/open-telemetry/opentelemetry-proto/blob/v0.14.0/opentelemetry/proto/metrics/v1/metrics.proto#L55-L58

Just need clarification, noticed while prepping a followup to this PR to use the new SDK metric types and can rectify it there.

Looks like the current types don't provide a schema url for the resource. I'll remove it and leave a TODO in the code.

actually it doesn't work like you're thinking. The resource schema url would live next to the resourcemetrics array, not within it. In any case, the types in this particular function are not otlp types so I renamed them to make it more obvious what they represent. You can look at the tests to see that the output is correct.

legendecas · 2022-03-22T17:17:12Z

The CI failures for sdk-node resource detectors should have been fixed by #2844.

dyladan · 2022-03-23T17:50:04Z

Browser test failure seems unrelated

legendecas · 2022-03-24T02:52:28Z

Browser test failure seems unrelated

@dyladan I've created an issue to track that #2852

dyladan requested a review from a team January 28, 2022 20:49

legendecas approved these changes Feb 6, 2022

View reviewed changes

experimental/tsconfig.json Outdated Show resolved Hide resolved

experimental/packages/proto/src/metrics/types.ts Outdated Show resolved Hide resolved

legendecas reviewed Feb 6, 2022

View reviewed changes

experimental/packages/proto/src/common/internal.ts Outdated Show resolved Hide resolved

legendecas linked an issue Feb 14, 2022 that may be closed by this pull request

[Proposal] Extract basic packages for otlp exporters #2665

Closed

dyladan mentioned this pull request Feb 22, 2022

Refactor metrics export data model to match proto #2775

Closed

dyladan marked this pull request as draft February 23, 2022 16:05

dyladan mentioned this pull request Feb 24, 2022

OTLP JSON exporter cannot be released #2804

Open

dyladan force-pushed the proto-json branch 3 times, most recently from 650d6ec to 0243756 Compare February 25, 2022 14:10

dyladan removed a link to an issue Feb 25, 2022

[Proposal] Extract basic packages for otlp exporters #2665

Closed

dyladan marked this pull request as ready for review February 28, 2022 18:06

dyladan force-pushed the proto-json branch from 070faf4 to 204fcc2 Compare February 28, 2022 18:11

legendecas reviewed Mar 1, 2022

View reviewed changes

experimental/packages/otlp-transformer/descriptor.json Outdated Show resolved Hide resolved

MSNev reviewed Mar 2, 2022

View reviewed changes

experimental/packages/otlp-transformer/src/common/internal.ts Outdated Show resolved Hide resolved

legendecas reviewed Mar 15, 2022

View reviewed changes

feat(proto): add @opentelemetry/proto package with hand-rolled proto …

67aa2f4

…json transformation

dyladan force-pushed the proto-json branch from 453f2cc to 67aa2f4 Compare March 15, 2022 09:06

blumamir approved these changes Mar 15, 2022

View reviewed changes

experimental/packages/otlp-transformer/src/common/internal.ts Outdated Show resolved Hide resolved

experimental/packages/otlp-transformer/src/common/types.ts Show resolved Hide resolved

experimental/packages/otlp-transformer/src/trace/internal.ts Outdated Show resolved Hide resolved

dyladan added 4 commits March 15, 2022 13:15

chore: review comments

b1b02df

chore: do not export metrics for now

512cba1

fix: use proper iteration

ee847ec

chore: update submodules after compile

c647f16

seemk approved these changes Mar 21, 2022

View reviewed changes

Merge branch 'main' into proto-json

158401e

vmarchaud approved these changes Mar 21, 2022

View reviewed changes

dyladan added 2 commits March 21, 2022 16:27

chore: fix browser test (mismatched version)

7401a5c

chore: fix tsconfigs for new build types

7f62328

seemk reviewed Mar 21, 2022

View reviewed changes

dyladan added 2 commits March 22, 2022 08:35

chore: more obvious names for intermediate types

8b525cf

Merge branch 'main' into proto-json

f537dfc

Flarna added 2 commits March 22, 2022 18:30

Merge branch 'main' into proto-json

0c03d86

Merge branch 'main' into proto-json

5be5d87

dyladan changed the title ~~feat(proto): add @opentelemetry/proto package with hand-rolled transformation~~ feat(proto): add @opentelemetry/otlp-transformer package with hand-rolled transformation Mar 23, 2022

dyladan added the enhancement New feature or request label Mar 23, 2022

dyladan merged commit 549bd5b into open-telemetry:main Mar 23, 2022

dyladan deleted the proto-json branch March 23, 2022 17:51

pichlermarc mentioned this pull request Mar 31, 2022

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

Merged

7 tasks

markandrus mentioned this pull request Apr 6, 2022

Compile protos statically instead of importing them dynamically #2887

Closed

pichlermarc mentioned this pull request Apr 26, 2022

feat(exporters): update proto version and use otlp-transformer #2929

Merged

9 tasks

gmreads mentioned this pull request Feb 23, 2023

Incorrect SpanKind sent by oltp-transformer #3631

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(proto): add @opentelemetry/otlp-transformer package with hand-rolled transformation #2746

feat(proto): add @opentelemetry/otlp-transformer package with hand-rolled transformation #2746

dyladan commented Jan 28, 2022 •

edited

Loading

codecov bot commented Jan 28, 2022 •

edited

Loading

legendecas left a comment

morigs commented Feb 21, 2022

dyladan commented Feb 22, 2022

dyladan commented Feb 23, 2022

dyladan commented Feb 25, 2022

dyladan commented Feb 25, 2022

vmarchaud commented Feb 25, 2022

dyladan commented Feb 28, 2022

legendecas commented Feb 28, 2022 •

edited

Loading

legendecas commented Mar 1, 2022

vmarchaud commented Mar 1, 2022

dyladan commented Mar 1, 2022

dyladan commented Mar 1, 2022

legendecas commented Mar 1, 2022

dyladan commented Mar 1, 2022

legendecas left a comment

dyladan commented Mar 15, 2022

seemk left a comment

seemk Mar 21, 2022

dyladan Mar 22, 2022

dyladan Mar 22, 2022

dyladan Mar 22, 2022 •

edited

Loading

legendecas commented Mar 22, 2022

dyladan commented Mar 23, 2022

legendecas commented Mar 24, 2022

feat(proto): add @opentelemetry/otlp-transformer package with hand-rolled transformation #2746

feat(proto): add @opentelemetry/otlp-transformer package with hand-rolled transformation #2746

Conversation

dyladan commented Jan 28, 2022 • edited Loading

codecov bot commented Jan 28, 2022 • edited Loading

Codecov Report

legendecas left a comment

Choose a reason for hiding this comment

morigs commented Feb 21, 2022

dyladan commented Feb 22, 2022

dyladan commented Feb 23, 2022

dyladan commented Feb 25, 2022

dyladan commented Feb 25, 2022

vmarchaud commented Feb 25, 2022

dyladan commented Feb 28, 2022

legendecas commented Feb 28, 2022 • edited Loading

legendecas commented Mar 1, 2022

vmarchaud commented Mar 1, 2022

dyladan commented Mar 1, 2022

dyladan commented Mar 1, 2022

legendecas commented Mar 1, 2022

dyladan commented Mar 1, 2022

legendecas left a comment

Choose a reason for hiding this comment

dyladan commented Mar 15, 2022

seemk left a comment

Choose a reason for hiding this comment

seemk Mar 21, 2022

Choose a reason for hiding this comment

dyladan Mar 22, 2022

Choose a reason for hiding this comment

dyladan Mar 22, 2022

Choose a reason for hiding this comment

dyladan Mar 22, 2022 • edited Loading

Choose a reason for hiding this comment

legendecas commented Mar 22, 2022

dyladan commented Mar 23, 2022

legendecas commented Mar 24, 2022

dyladan commented Jan 28, 2022 •

edited

Loading

codecov bot commented Jan 28, 2022 •

edited

Loading

legendecas commented Feb 28, 2022 •

edited

Loading

dyladan Mar 22, 2022 •

edited

Loading