feat: browser support for exporter-trace-otlp-proto #3208

pkanal · 2022-08-26T19:05:07Z

Which problem is this PR solving?

Provide the option of exporting traces using protobuf from the browser. Previously this package only supported Node, since this package now uses protobuf-js it is possible to send protobuf from the browser.

Fixes #3118

TODOs

Compression options
Add karma config for tests
Add unit tests
Add example of using the proto exporter in the browser
Finish up tests listed below

Short description of the changes

Added browser exporter for the proto exporter package
Added browser exporter for trace proto exporter package
Added example using the trace proto exporter package

Type of change

Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Send traces in the browser using the proto exporter
Node proto exporter still works as expected
Env variables are supported
Existing config is supported

Checklist:

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

scheler · 2022-10-14T23:40:42Z

experimental/packages/otlp-proto-exporter-base/src/platform/browser/util.ts

+        collector.timeoutMillis,
+        onSuccess,
+        onError
+      );


Not related to this PR, but I wonder why the headers and timeout are set only in the browser case. In the node case, are the headers passed in env variables OTEL_EXPORTER_OTLP_HEADERS and OTEL_EXPORTER_OTLP_TRACES_HEADERS ignored?

scheler · 2022-10-14T23:43:19Z

examples/opentelemetry-web/examples/fetch/index.js

-const { ZoneContextManager } = require( '@opentelemetry/context-zone');
-const { B3Propagator } = require( '@opentelemetry/propagator-b3');
-const { registerInstrumentations } = require( '@opentelemetry/instrumentation');
+const { context, trace } = require("@opentelemetry/api");


Looks like this file has only formatting changes. If so, can you submit them in a separate PR and revert them here?

We also prefer single quotes over double quotes in the repo, but there is just no eslint rule for *.js files at the moment. For this reason, and for making this PR easier to review, undoning the formatting changes would be greatly appreciated 🙂

scheler · 2022-10-15T00:00:15Z

...ental/packages/otlp-proto-exporter-base/src/platform/browser/OTLPProtoExporterBrowserBase.ts

+  OTLPExporterError,
+  OTLPExporterConfigBase
+} from '@opentelemetry/otlp-exporter-base';
+import { send } from './util';


I wonder what was the reason send function was moved into a separate util, and not OTLPProtoExporterBrowserBase itself. Are there any benefits to doing it?

See https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/otlp-grpc-exporter-base/src/OTLPGRPCExporterNodeBase.ts#L78-L79 for the reason for Node.js grpc instrumentation.

I think it is fine to move the util function as a member method for the web, as the send function is accessing many members of the collector and not depends on external modules.

scheler · 2022-10-15T00:03:01Z

examples/opentelemetry-web/examples/fetchProtoExporter/index.html

@@ -0,0 +1,20 @@
+<!DOCTYPE html>


You might want to call this folder fetch-proto to keep it simple

pichlermarc

This is great, thank you for working on this 🙂
Just a few smaller comments. 🙂

examples/opentelemetry-web/examples/fetchProtoExporter/index.js

examples/opentelemetry-web/package.json

pichlermarc · 2022-10-18T07:06:26Z

examples/opentelemetry-web/examples/fetch/index.js

-const { ZoneContextManager } = require( '@opentelemetry/context-zone');
-const { B3Propagator } = require( '@opentelemetry/propagator-b3');
-const { registerInstrumentations } = require( '@opentelemetry/instrumentation');
+const { context, trace } = require("@opentelemetry/api");


We also prefer single quotes over double quotes in the repo, but there is just no eslint rule for *.js files at the moment. For this reason, and for making this PR easier to review, undoning the formatting changes would be greatly appreciated 🙂

experimental/packages/otlp-exporter-base/src/platform/browser/OTLPExporterBrowserBase.ts

...ental/packages/otlp-proto-exporter-base/src/platform/browser/OTLPProtoExporterBrowserBase.ts

experimental/packages/otlp-proto-exporter-base/src/platform/browser/util.ts

…exporter-browser

codecov · 2022-11-08T20:52:51Z

Codecov Report

Merging #3208 (426f321) into main (5127371) will decrease coverage by 0.02%.
The diff coverage is n/a.

❗ Current head 426f321 differs from pull request most recent head 9429864. Consider uploading reports for the commit 9429864 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3208      +/-   ##
==========================================
- Coverage   93.80%   93.79%   -0.02%     
==========================================
  Files         249      249              
  Lines        7640     7640              
  Branches     1589     1589              
==========================================
- Hits         7167     7166       -1     
- Misses        473      474       +1

Impacted Files	Coverage Δ
...-otlp-proto/src/platform/node/OTLPTraceExporter.ts	`100.00% <ø> (ø)`
packages/opentelemetry-resources/src/Resource.ts	`100.00% <ø> (ø)`
...-trace-base/src/platform/node/RandomIdGenerator.ts	`87.50% <0.00%> (-6.25%)`	⬇️

…exporter-browser

scheler · 2022-11-12T06:24:55Z

@pichlermarc @legendecas @dyladan could one of you help check what the unit tests are failing for? I only see "Error: Process completed with exit code 3." at the end of each tests run, but it's not clear what error the process encountered. In my local setup I see failure in api/test/tree-shaking/tree-shaking.test.ts (even though this PR has made no changes to the api), but it's not clear if the unit tests in github CI are failing for the same reason. Thanks for looking into this.

legendecas · 2022-11-14T08:15:01Z

@scheler I'll take a look at the CI problem.

MSNev · 2022-12-14T21:33:15Z

experimental/packages/exporter-trace-otlp-proto/src/platform/browser/OTLPTraceExporter.ts

+  }
+
+  getDefaultUrl(config: OTLPExporterConfigBase): string {
+    return typeof config.url === 'string'


Suggestion
This is a little more readable, performant (and minifiable) by rewriting as

let endpoint = config.url; if (!endpoint || endpoint !== 'string') { let env = getEnv(); let tracesEndpoint = env.OTEL_EXPORTER_OTLP_TRACES_ENDPOINT; if (tracesEndpoint) { endpoint = appendRootPathToUrlIfNeeded(tracesEndpoint); } else { let otlpEndpoint = env.OTEL_EXPORTER_OTLP_ENDPOINT; if (otlpEndpoint) { endpoint = appendResourcePathToUrl(otlpEndpoint, DEFAULT_COLLECTOR_RESOURCE_PATH); } } } return endpoint || DEFAULT_COLLECTOR_URL;

As well as less error prone for someone coming looking at this after you

@MSNev The function getDefaultUrl is written this way in multiple packages. I suggest we take this up in a separate PR since it's unrelated to this PR.

Can we raise an issue for this then, otherwise it will not get done. This can include the suggestion I have above as the implementation.

dyladan · 2022-12-14T21:20:56Z

...ental/packages/otlp-proto-exporter-base/src/platform/browser/OTLPProtoExporterBrowserBase.ts

+/**
+ * Collector Exporter abstract base class
+ */
+export abstract class OTLPProtoExporterBrowserBase<


Usually the node and browser exports would have the same name and be chosen at compile time. Is there a reason you've chosen to give it a separate name here?/

Yeah, we have a section about this rule: https://github.com/open-telemetry/opentelemetry-js/blob/main/CONTRIBUTING.md#platform-conditional-exports.

If it is needed to be exported under different names, it can be placed outside platform/ to avoid unexpected missing exports.

Thanks for pointing it out. I will make the changes.

@dyladan @legendecas this doesn't seem to be straightforward. The classes are named differently in platform/ in the package otlp-exporter-base and they follow different interface - that is, one has _headers private member and another has headers public member. I am running into errors using the same name in the platform/ of package otlp-proto-exporter-base. The errors manifest in the package exporter-trace-otlp-proto. It looks like the rule of using the same name for classes in platform/ is not followed already, so I suggest we take this up in a separate PR as it requires changes in multiple packages and requires more care. Let me know what you think.

@dyladan @legendecas can you please comment on the above observation?

Sorry for the delayed response. I thought I've hit the comment button.

Yeah, it is true that the rule has already been violated in the package otlp-exporter-base. I think it is fine to defer the renaming since it would not be an easy work without the refactoring in the otlp-exporter-base first.

tsconfig.base.esm.json

MSNev

Left suggestion for getDefaultUrl()

…try-js into proto-exporter-browser

pichlermarc

Thank you for moving this along 🙂

It looks like the opentelemetry-web is missing the proto exporter dependency.
I was also wondering about some packaging details - see comments below 🙂

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

experimental/packages/exporter-trace-otlp-proto/test/browser/CollectorTraceExporter.test.ts

experimental/packages/otlp-proto-exporter-base/package.json

experimental/packages/otlp-exporter-base/src/index.ts

…ollectorTraceExporter.test.ts Co-authored-by: Marc Pichler <[email protected]>

that will now be used for the browser case as well.

pichlermarc

Thank you for the great work on this @pkanal and @scheler 💯
This is no doubt going to help out a lot of people. 🙂

legendecas · 2023-01-17T08:23:17Z

Thank you for working on this!

scheler · 2023-01-19T19:00:13Z

@open-telemetry/javascript-maintainers can you review and merge this PR? It has a few approvals already.

pkanal changed the title ~~Proto exporter browser~~ [WIP] Browser support for exporter-trace-otlp-proto Aug 26, 2022

pkanal added 7 commits October 3, 2022 15:35

add node & browser platform for exporter-trace-otlp-proto

915ce52

add browser support for proto base exporter

87327f0

add base browser class with xhr support

b7c8c19

add browser class for proto trace exporter

1fec3b3

slight tweaks to make it work

4214129

send data as blob to avoid making sync xhr requests

d33f9bb

fix lint

3dd4e17

pkanal force-pushed the proto-exporter-browser branch from b4effd1 to 3dd4e17 Compare October 3, 2022 19:42

remove console.logs and add browser proto example

35c193f

pkanal mentioned this pull request Oct 11, 2022

Protobuf trace exporter for browser #3118

Closed

cleanup and start adding tests

5ed212d

pkanal marked this pull request as ready for review October 12, 2022 14:53

pkanal requested a review from a team October 12, 2022 14:53

pkanal changed the title ~~[WIP] Browser support for exporter-trace-otlp-proto~~ feat: browser support for exporter-trace-otlp-proto Oct 12, 2022

scheler reviewed Oct 15, 2022

View reviewed changes

pichlermarc reviewed Oct 18, 2022

View reviewed changes

scheler added 3 commits November 7, 2022 14:09

Merge branch 'main' of github.com:pkanal/opentelemetry-js into proto-…

933d37e

…exporter-browser

Undo formatting changes

9953561

exporter-trace-otlp-proto: fix compile errors

5e87c85

scheler and others added 5 commits November 9, 2022 14:25

Misc updates from review comments

1616d2b

Merge branch 'main' of github.com:pkanal/opentelemetry-js into proto-…

e3043b0

…exporter-browser

Adding changelog entry

0020c98

Merge branch 'open-telemetry:main' into proto-exporter-browser

6125ec2

Reverting format changes not needed

c420fd0

legendecas mentioned this pull request Nov 14, 2022

test(api): disable module concatenation in tree-shaking test #3409

Merged

3 tasks

Merge branch 'main' into proto-exporter-browser

f260db8

Using import from top level folder

519fa7c

MSNev reviewed Dec 14, 2022

View reviewed changes

dyladan reviewed Dec 14, 2022

View reviewed changes

MSNev reviewed Dec 14, 2022

View reviewed changes

tsconfig.base.esm.json Outdated Show resolved Hide resolved

MSNev approved these changes Dec 14, 2022

View reviewed changes

scheler and others added 4 commits December 28, 2022 08:24

Merge branch 'open-telemetry:main' into proto-exporter-browser

5f67e56

Trigger Build

ad2aa0e

Merge branch 'proto-exporter-browser' of github.com:pkanal/openteleme…

e57f0f2

…try-js into proto-exporter-browser

Merge branch 'main' into proto-exporter-browser

b26fd7f

pichlermarc reviewed Jan 2, 2023

View reviewed changes

scheler and others added 8 commits January 2, 2023 13:59

Update experimental/packages/exporter-trace-otlp-proto/test/browser/C…

f421989

…ollectorTraceExporter.test.ts Co-authored-by: Marc Pichler <[email protected]>

Remove trailing comma

8064d5b

Remove blank line to fix lint error

2678ff3

Fixes based on testing opentelemetry-web/fetch-proto

c898ea2

Add additional missing export

d48503a

Skip hex conversion of traceId for the protobuf

a800550

Add esm/esnext builds for the proto packages

598bc56

that will now be used for the browser case as well.

Merge branch 'main' into proto-exporter-browser

5596d13

pichlermarc approved these changes Jan 16, 2023

View reviewed changes

legendecas approved these changes Jan 17, 2023

View reviewed changes

legendecas and others added 2 commits January 17, 2023 16:23

Merge branch 'main' into proto-exporter-browser

1788269

Merge branch 'main' into proto-exporter-browser

2e0c4ec

Trigger Build

9429864

legendecas merged commit 9589d54 into open-telemetry:main Jan 20, 2023

Flarna mentioned this pull request Jan 25, 2023

fix: remove JSON syntax error and regenerate tsconfig files #3566

Merged

SimenB mentioned this pull request Jan 31, 2023

@opentelemetry/otlp-exporter-base TS error in Node environment #3580

Open

pkanal mentioned this pull request May 9, 2023

REQUEST: New membership for pkanal open-telemetry/community#1472

Closed

6 tasks

pichlermarc mentioned this pull request Jul 25, 2023

fix: Replace require with import #4015

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: browser support for exporter-trace-otlp-proto #3208

feat: browser support for exporter-trace-otlp-proto #3208

pkanal commented Aug 26, 2022 •

edited

Loading

scheler Oct 14, 2022

scheler Oct 14, 2022

pichlermarc Oct 18, 2022

scheler Oct 15, 2022

legendecas Nov 21, 2022

scheler Oct 15, 2022

pichlermarc left a comment

pichlermarc Oct 18, 2022

codecov bot commented Nov 8, 2022 •

edited

Loading

scheler commented Nov 12, 2022

legendecas commented Nov 14, 2022

MSNev Dec 14, 2022 •

edited

Loading

scheler Dec 28, 2022

MSNev Jan 9, 2023

dyladan Dec 14, 2022

legendecas Dec 15, 2022

scheler Dec 15, 2022

scheler Dec 28, 2022

scheler Jan 6, 2023

legendecas Jan 17, 2023

MSNev left a comment

pichlermarc left a comment

pichlermarc left a comment

legendecas commented Jan 17, 2023

scheler commented Jan 19, 2023

feat: browser support for exporter-trace-otlp-proto #3208

feat: browser support for exporter-trace-otlp-proto #3208

Conversation

pkanal commented Aug 26, 2022 • edited Loading

Which problem is this PR solving?

TODOs

Short description of the changes

Type of change

How Has This Been Tested?

Checklist:

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

Choose a reason for hiding this comment

codecov bot commented Nov 8, 2022 • edited Loading

Codecov Report

scheler commented Nov 12, 2022

legendecas commented Nov 14, 2022

MSNev Dec 14, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSNev left a comment

Choose a reason for hiding this comment

pichlermarc left a comment

Choose a reason for hiding this comment

pichlermarc left a comment

Choose a reason for hiding this comment

legendecas commented Jan 17, 2023

scheler commented Jan 19, 2023

pkanal commented Aug 26, 2022 •

edited

Loading

codecov bot commented Nov 8, 2022 •

edited

Loading

MSNev Dec 14, 2022 •

edited

Loading