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

GRPC-JS Error: Metadata key contains illegal characters #527

Closed
daaain opened this issue Jul 4, 2019 · 7 comments · Fixed by googleapis/gax-nodejs#532
Closed

GRPC-JS Error: Metadata key contains illegal characters #527

daaain opened this issue Jul 4, 2019 · 7 comments · Fixed by googleapis/gax-nodejs#532
Assignees
Labels
api: logging Issues related to the googleapis/nodejs-logging API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@daaain
Copy link

daaain commented Jul 4, 2019

Environment details

  • OS: Debian (using node:10.15.0 Docker image)
  • Node.js version: v10.15.0
  • npm version: 6.4.1 (using yarn 1.12.3 though)
  • @google-cloud/logging-winston version: 1.1.0

Steps to reproduce

  1. Send logs via this logger
  2. Wait
  3. Get error caught by a reporting library, like Sentry in our case

I've been seeing this error coming up sporadically, with different characters in the key each time:

Error: Metadata key "���é" contains illegal characters
  File "app:///../node_modules/@grpc/grpc-js/build/src/client.js", line 101, col 45, in Http2CallStream.call.on
    const error = Object.assign(new Error(status.details), status);
  File "events.js", line 187, col 15, in Http2CallStream.emit
  File "domain.js", line 459, col 23, in Http2CallStream.EventEmitter.emit
  File "app:///../node_modules/@grpc/grpc-js/build/src/call-stream.js", line 71, col 22, in process.nextTick
    this.emit('status', status);
  File "internal/process/next_tick.js", line 61, col 11, in process._tickCallback

It only appeared once we had a deployment with logging using this library, so I'm pretty sure it's the logging transport, but can't be 100% as the stack trace starts in the grpc-js dependency.

Since I don't know where it is coming from, I don't really know how debug it. I'm guessing because the work is offloaded to the next tick I wouldn't even be able to catch the library error in my client code either 🤷‍♀

I found this related upstream issue: grpc/grpc-node#519

@DominicKramer
Copy link
Contributor

Thank you for opening this issue. I'm transferring the issue to nodejs-logging since interactions with the logging API are not done by this library but are instead done using the nodejs-logging library. As such, I don't think this issue is specific to the nodejs-logging-winston library.

@DominicKramer DominicKramer transferred this issue from googleapis/nodejs-logging-winston Jul 8, 2019
@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Jul 8, 2019
@bcoe bcoe added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p2 Moderately-important priority. Fix may not be included in next release. labels Jul 9, 2019
@yoshi-automation yoshi-automation removed the triage me I really want to be triaged. label Jul 9, 2019
@callmehiphop
Copy link
Contributor

Hi @daaain can I ask what version of google-gax is installed in your project? I think a fix went out for this in v1.1.12.

@daaain
Copy link
Author

daaain commented Jul 9, 2019

@callmehiphop just had a look, apparently it was an older version according to yarn:

google-gax@^1.0.0:
  version "1.1.2"
  resolved "https://registry.yarnpkg.com/google-gax/-/google-gax-1.1.2.tgz#e526c375275759696ac4a0424bd5ce476a66729b"
  integrity sha512-kl/7U5x0Db9TwY4qYKomyBjKAgy3ztb1iqpx/wC84CtUVMNa1JNyPVlVdjgJS2JgOUF0yxMp9z6amfvss6tSIA==

I'll bump the version and report back!

@daaain
Copy link
Author

daaain commented Jul 9, 2019

Wait, it looks like that fix was in 1.1.2 after all: https://github.com/googleapis/gax-nodejs/releases/tag/v1.1.2

The thing I noticed though is that it fixes encoding for values, but not keys, so maybe that's the issue?

I honestly have no idea where these non-ASCII characters come from and not sure if URL encoding them would help, but I guess it's still better to have normalised strings for keys than the library exploding 🤷‍♀

Edit: oh no, it's only the test which tests for values only, but the actual method does encode the keys too...

@daaain
Copy link
Author

daaain commented Jul 9, 2019

Oh, and also noticed that googleapis/nodejs-datastore#415 and googleapis/nodejs-pubsub#667 and googleapis/nodejs-pubsub#680 are similar, but wondering if the issue is solved for those guys after all?

@callmehiphop
Copy link
Contributor

@daaain oh whoops, that was a typo on my part, 1.1.2 is correct. @alexander-fenster any insight here?

alexander-fenster added a commit to googleapis/gax-nodejs that referenced this issue Jul 12, 2019
This fix includes grpc/grpc-node#962 which is a work around for a bad metadata keys problem that many customer see across several libraries (googleapis/nodejs-datastore#415, googleapis/nodejs-logging#527, googleapis/nodejs-pubsub#667 are just a few).
JustinBeckwith pushed a commit to googleapis/gax-nodejs that referenced this issue Jul 12, 2019
This fix includes grpc/grpc-node#962 which is a work around for a bad metadata keys problem that many customer see across several libraries (googleapis/nodejs-datastore#415, googleapis/nodejs-logging#527, googleapis/nodejs-pubsub#667 are just a few).
@alexander-fenster
Copy link
Contributor

Hi @daaain,

We have released google-gax v1.1.5 that pulls @grpc/grpc-js v0.5.2 in which the invalid headers (that are coming from Node.js http2 module) are printed in a warning, but do not throw exception. Please upgrade your dependencies, it should fix the problem for you.

@google-cloud-label-sync google-cloud-label-sync bot added the api: logging Issues related to the googleapis/nodejs-logging API. label Jan 31, 2020
@JustinBeckwith JustinBeckwith self-assigned this Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: logging Issues related to the googleapis/nodejs-logging API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants