Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exporter environment configuration #1676

Closed
dyladan opened this issue Nov 11, 2020 · 7 comments · Fixed by #2100
Closed

Exporter environment configuration #1676

dyladan opened this issue Nov 11, 2020 · 7 comments · Fixed by #2100
Assignees

Comments

@dyladan
Copy link
Member

dyladan commented Nov 11, 2020

Implement the environment variable specification for exporter configuration.

Please use the getEnv('NAME') function exported by core instead of directly using process.env.NAME.
Spec: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/sdk-environment-variables.md#exporter-selection

@vmarchaud
Copy link
Member

Moving the discussion over here (started by @jtmalinowski here):

OTEL_TRACES_EXPORTER - kind of tricky, default is otlp for which JSON (aka web) support is experimental, so doesn't seem suitable for 1.0

I agree with that, i would like to add that if we want to support other exporters we would need to bring them as dependencies for sdk which seems quite negative in terms of dependencies size:

  • the spec requires to have zipkin send as protobuf but we only support JSON, if we did support protobuf that would requires to have @grpc/proto-loader + protobufjs dependency.
  • the jeager exporter use thrift currently but the spec says it should be using grpc, which have the same problem as zipkin
  • finally otlp, from the spec it looks like the protocol should be configurable, so does that means we need to bring all 3 exporters to support the 3 protocols ?

I would be in favor of only supporting the otlp with http/json protocol for now. I don't think the fact that the exporter itself is experimental is a problem since the goal is to GA the sdk itself and not necessarily all components that it can be used with.

@vmarchaud
Copy link
Member

I'll also add that the spec doesn't specify which span processor should be configured (even though it defines batch processor's configuration) so i assumes it should be the batch ?

@vmarchaud
Copy link
Member

Requested clarification from the spec: open-telemetry/opentelemetry-specification#1622

However it seems that Ruby just check if dependency are installed (https://github.com/open-telemetry/opentelemetry-ruby/pull/604/files#diff-e6a05d2b9d8d72a4c1ab589e75a9aa34e484e073af6443d428929c0c554f93eaR201), it would be easily be possible for the node SDK (harder for the browser though) with optional dependencies, WDYT ?

@jtmalinowski
Copy link
Contributor

Just an opinion but I like supporting OTEL_TRACES_EXPORTER in NodeJS and not supporting it in the browser. On the server-side it potentially allows configuring exporting by devops without touching the application code, on the client-side I can't see any significant benefits.

@dyladan
Copy link
Member Author

dyladan commented Apr 20, 2021

I agree that dynamically configuring the exporter in the browser seems like extremely questionable value.

@obecny
Copy link
Member

obecny commented Apr 28, 2021

not sure if this is already not too late, but I was proposing long time ago we split each exporter into at least 2 things, "converter" and "transporter" with exporter collector it will happened quite soon - the function for converting will be exported into new package right ?.

I can imagine we could have some builder pattern that could build the exporter pipeline like this.

const exporter = (new ZipkinExporter()).addTransport(new HttpTransport())
// or
const exporter = (new ZipkinExporter()).addTransport(new XHRTransport())
// or
const exporter = (new ZipkinExporter()).addTransport(new FetchTransport())
// or
const exporter = (new ZipkinExporter()).addTransport(new GrpcTransport())
// and so on
const exporter = (new CollectorExporter()).addTransport(new XHRTransport())
// and so on
const exporter = (new JaegerExporter()).addTransport(new HttpTransport())

@dyladan
Copy link
Member Author

dyladan commented Apr 28, 2021

This seems like a big change for limited value since most exporters have 1 preferred transport

vmarchaud added a commit to vmarchaud/opentelemetry-js that referenced this issue May 2, 2021
vmarchaud added a commit to vmarchaud/opentelemetry-js that referenced this issue May 2, 2021
vmarchaud added a commit to vmarchaud/opentelemetry-js that referenced this issue May 8, 2021
vmarchaud added a commit to vmarchaud/opentelemetry-js that referenced this issue May 11, 2021
vmarchaud added a commit that referenced this issue May 12, 2021
Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>
Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this issue Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants