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

feat(otel-tasks-runner): lazy load exporter libraries and make them optl #6

Merged
merged 2 commits into from
Oct 10, 2022

Conversation

gibsonjoshua55
Copy link
Owner

This PR addresses Issue #5. It makes the OTLP HTTP and gRPC exporters optional peer dependencies. The dependencies are only required if you configure the tasks runner to use those specific exporters.

Breaking Changes

This PR changes the default exporter from otlp-grpc to console. This way if you do not specify a specific exporter, no additional exporter packages will be required to be installed.

@gibsonjoshua55
Copy link
Owner Author

@Djiit Let me know if this does what you were looking for!

@Djiit
Copy link
Contributor

Djiit commented Oct 9, 2022

Yup, something like this :) Thank you

Reading the doc, I think you have to declare the 2 dependencies in peerDependencies and in peerDepencenciesMeta (see https://docs.npmjs.com/cli/v8/configuring-npm/package-json#peerdependenciesmeta)

@gibsonjoshua55
Copy link
Owner Author

@Djiit Nx automatically defines those for us when we build the package with the buildableProjectDepsInPackageJsonType option.

"build": {
      "executor": "@nrwl/js:swc",
      ...
        "buildableProjectDepsInPackageJsonType": "peerDependencies"
      }
    }

This is what the generated package json looks like when built:

"peerDependencies": {
    "@nrwl/devkit": "^13.10.0",
    "nx": "^13.10.0",
    "@grpc/grpc-js": "^1.3.7",
    "@opentelemetry/exporter-trace-otlp-grpc": "^0.27.0",
    "@opentelemetry/sdk-node": "^0.26.0",
    "@opentelemetry/sdk-trace-base": "^1.0.0",
    "@opentelemetry/api": "^1.0.3",
    "@opentelemetry/core": "^1.0.0",
    "@opentelemetry/resources": "^1.0.0",
    "@opentelemetry/semantic-conventions": "^1.0.0",
    "@opentelemetry/exporter-trace-otlp-http": "^0.27.0",
    "rxjs": "^6.6.7"
  },
  "dependencies": {
    "@swc/helpers": "~0.3.3"
  },
  "devDependencies": {},
  "peerDependenciesMeta": {
    "@opentelemetry/exporter-trace-otlp-grpc": {
      "optional": true
    },
    "@opentelemetry/exporter-trace-otlp-http": {
      "optional": true
    }
  },

I don't think we need to manually define those dependencies. The NX packages are manually defined so that we can make their version requirements more flexible than what we've defined in the root package.json, but no changes are needed for those packages.

@Djiit
Copy link
Contributor

Djiit commented Oct 9, 2022

Alright, lgtm :)

On a side note, may I ask why the swc/helpers is a direct dependency ?

@gibsonjoshua55 gibsonjoshua55 force-pushed the feat/optional-exporters branch from c6fa0e7 to 7cd9686 Compare October 9, 2022 18:45
@gibsonjoshua55
Copy link
Owner Author

Good question. Looks like the SWC compiler was configured to use externally packaged helpers rather than inline them. Looking at their docs, that's really only helpful in bundled apps.

I've updated the compiler settings to use inline helpers and removed the dependency.

@gibsonjoshua55 gibsonjoshua55 merged commit 9739a2d into main Oct 10, 2022
@gibsonjoshua55 gibsonjoshua55 deleted the feat/optional-exporters branch October 10, 2022 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants