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

Add support for @grpc/grpc-js 1.x #1135

Closed
sk- opened this issue Jun 3, 2020 · 8 comments · Fixed by #1201
Closed

Add support for @grpc/grpc-js 1.x #1135

sk- opened this issue Jun 3, 2020 · 8 comments · Fixed by #1201
Assignees

Comments

@sk-
Copy link

sk- commented Jun 3, 2020

Is your feature request related to a problem? Please describe.
I want to trace the execution of Firebase Functions using firestore. However the firestore clients use grpc-js 0.7.4, due to dependencies on google-gax 1.13.0.

Describe the solution you'd like
Support for grpc-js 0.7.4

Describe alternatives you've considered
I checked whether it would be possible to update the firestore client to the latest version, but that still uses google-gax 1.13.0, which depends on the grpc-js version mentioned earlier.

I also created googleapis/nodejs-firestore#1109 asking to update the dependency.

Additional context
Add any other context or screenshots about the feature request here.

@dyladan
Copy link
Member

dyladan commented Jun 3, 2020

maybe @markwolff can weigh in here as he is the original plugin author. I assume there is some reason he only decided to support 1.x. This definitely depends on the level of effort needed to support it as 0.x is a pre-release version of grpc and I am hesitant to spend too much effort supporting old pre-release versions of modules.

@markwolff
Copy link
Member

grpc-js is a re-write of grpc using pure javascript instead of relying on native modules, and was not recommended for production until a month ago with its 1.x release.

The action item here would be to write a new plugin for grpc-js. Only grpc-js@1.x should be supported, but judging from the release notes, there aren't any breaking changes from 0.x...1.x so both should be compatible.

https://www.npmjs.com/package/@grpc/grpc-js
https://www.npmjs.com/package/grpc

@dyladan
Copy link
Member

dyladan commented Jun 3, 2020

If only 1.x is supported, it will fail to install on 0.x. We currently don't have a mechanism to force the installation to succeed on an incompatible version. Maybe we should? Obviously we would want to name the option something scary that makes it clear this is at your own risk.

@markwolff
Copy link
Member

@dyladan I am confused. Are you referring to the plugin loader?

@dyladan
Copy link
Member

dyladan commented Jun 3, 2020

Yes.

Only grpc-js@1.x should be supported, but judging from the release notes, there aren't any breaking changes from 0.x...1.x so both should be compatible.

If we create a plugin for grpc-js which only contains 1.x in the supported versions matrix, the plugin loader will not load it if you are using a version of grpc-js prior to 1.x

@markwolff markwolff changed the title Add support for grpc 0.7.4 Add support for @grpc/grpc-js 0.7.4 Jun 3, 2020
@markwolff
Copy link
Member

markwolff commented Jun 3, 2020

Makes sense. @sk- if you want to contribute this you are welcome to. Else I can take this when I have some cycles.

@dyladan Do you think this plugin should live in this repo or live in contrib? It seems to be the supported grpc lib going forward now, and so I think it should take the place as the "grpc plugin" for this repo. The existing one could be moved to contrib and potentially become non-default as well.

@dyladan
Copy link
Member

dyladan commented Jun 3, 2020

what do you mean "supported grpc lib"? I admit to being a little out of the loop on grpc related topics.

@markwolff
Copy link
Member

markwolff commented Jun 3, 2020

A quick overview:

  • grpc: grpc package which has existed for years. Written with native c++ addons. Stable, but doesn't seem to be getting updates anymore (past 6 months). It probably will get updated if the grpc protocol ever requires changes and so I don't think it is considered "unsupported". We have an opentelemetry plugin for this lib.
  • @grpc/grpc-js: Re-write of grpc to use pure JS (nodejs) instead of relying on c++ stuff. Was in beta until past few weeks. No opentelemetry plugin exists for this yet.

The code for both of these packages lives here and grpc-js is now more popular than its predecessor and is receiving regular updates. The feature gap between the two is here

@markwolff markwolff self-assigned this Jun 4, 2020
@markwolff markwolff changed the title Add support for @grpc/grpc-js 0.7.4 Add support for @grpc/grpc-js 1.x Jun 16, 2020
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this issue Dec 15, 2023
…1135 (open-telemetry#1137)

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
Co-authored-by: Valentin Marchaud <contact@vmarchaud.fr>
martinkuba pushed a commit to martinkuba/opentelemetry-js that referenced this issue Mar 13, 2024
…1135 (open-telemetry#1137)

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
Co-authored-by: Valentin Marchaud <contact@vmarchaud.fr>
martinkuba pushed a commit to martinkuba/opentelemetry-js that referenced this issue Mar 16, 2024
…1135 (open-telemetry#1137)

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
Co-authored-by: Valentin Marchaud <contact@vmarchaud.fr>
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.

3 participants