Protobuf trace exporter for browser

[martinkuba]

Is your feature request related to a problem? Please describe.

When exporting OTLP data from browsers, it is important to minimize the size of the payload. The existing JSON OTLP format can be very large due to long and repeated keys. This was discussed in this (rejected) PR open-telemetry/opentelemetry-proto#413. One outcome of that discussion was a recommendation to provide the option of exporting using protobuf rather than JSON.

Describe the solution you'd like

The current protobuf exporter works only with Node. The request is to make it work in browsers as well.

Describe alternatives you've considered

• JSON short keys (as discussed in this PR)
• gzip compression (this is rather an additional layer of optimization)

[dyladan]

I think actually the current one should work in browser it just needs to have tests added.

[dyladan]

The problem before was the .proto files and that's been sorted now that we ship compiled js files using protobufjs which supports browser

[martinkuba]

I tested it with the parcel example from the docs, and getting this error

OTLPProtoExporterNodeBase.ts:38 Uncaught TypeError: Class extends value undefined is not a constructor or null
    at ibkwK.@opentelemetry/api (OTLPProtoExporterNodeBase.ts:38:11)
    at newRequire (index.bac007fd.js:71:24)
    at localRequire (index.bac007fd.js:84:35)
    at 8cW21../OTLPProtoExporterNodeBase (index.ts:17:1)
    at newRequire (index.bac007fd.js:71:24)
    at localRequire (index.bac007fd.js:84:35)
    at kIJZb.@opentelemetry/core (OTLPTraceExporter.ts:24:1)
    at newRequire (index.bac007fd.js:71:24)
    at localRequire (index.bac007fd.js:84:35)
    at bcVjp../OTLPTraceExporter (index.ts:17:1)

There is a single exporter class in the package that extends OTLPProtoExporterNodeBase, unlike the http one that has a separate class for browser.

[pkanal]

I'm happy to take a look at this!

[dyladan]

I tested it with the parcel example from the docs, and getting this error

OTLPProtoExporterNodeBase.ts:38 Uncaught TypeError: Class extends value undefined is not a constructor or null
    at ibkwK.@opentelemetry/api (OTLPProtoExporterNodeBase.ts:38:11)
    at newRequire (index.bac007fd.js:71:24)
    at localRequire (index.bac007fd.js:84:35)
    at 8cW21../OTLPProtoExporterNodeBase (index.ts:17:1)
    at newRequire (index.bac007fd.js:71:24)
    at localRequire (index.bac007fd.js:84:35)
    at kIJZb.@opentelemetry/core (OTLPTraceExporter.ts:24:1)
    at newRequire (index.bac007fd.js:71:24)
    at localRequire (index.bac007fd.js:84:35)
    at bcVjp../OTLPTraceExporter (index.ts:17:1)

There is a single exporter class in the package that extends OTLPProtoExporterNodeBase, unlike the http one that has a separate class for browser.

Yeah there is actually nothing node specific there though as far as I'm aware now that proto stuff is gone. I think the problem will just be that we don't have browser targets and tsconfigs setup in the package.json. I'm pretty sure this is a packaging issue not a code issue.

[dyladan]

I could be wrong, and there may be some minor things to change, but overall I don't expect this to be a large body of work

[dyladan]

@pkanal any update on this?

[pkanal]

I started looking into this we need to add browser targets for otlp-exporter-base, otlp-proto-exporter-base and exporter-trace-otlp-proto. I have a branch on my fork where I'm trying to set this up so I'll push that up once I'm able to do a preliminary test. There might be a tweak we need to make to this require statement because I don't think its browser friendly.

[dyladan]

You're right it's not. I think we should be able to actually remove the async nature of that require entirely. It used to be required to do that so we wouldn't load grpc libraries before grpc instrumentation, but we now load instrumentation a different way that allows us to load it before setting up the SDK.

[github-actions]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[dyladan]

@pkanal any update here?

[pkanal]

I have a WIP PR here: #3208 and I should be able to get it into a reviewable state this week