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

test(exporter-logs-otlp-grpc): improve error reporting in particular test failure #4953

Merged
merged 1 commit into from
Aug 26, 2024

Conversation

trentm
Copy link
Contributor

@trentm trentm commented Aug 23, 2024

Exporter tests are currently failing with the very recent Node.js 22.7.0. We believe the failures are due to a Node 22.x regression for which there is an issue/PR/discussion.
This PR is about improving the test failure to be more helpful.

Before this change the test failure output was:

% cd experimental/packages/exporter-logs-otlp-grpc
% npm test
...
  1) OTLPLogExporter - node with TLS, without metadata, target https://localhost:1503
       "before all" hook for "test certs are valid":
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/trentm/tm/opentelemetry-js4/experimental/packages/exporter-logs-otlp-grpc/test/OTLPLogExporter.test.ts)
      at listOnTimeout (node:internal/timers:594:17)
      at processTimers (node:internal/timers:529:7)

  2) OTLPLogExporter - node without TLS, without metadata, target https://localhost:1503
       "before all" hook in "OTLPLogExporter - node without TLS, without metadata, target https://localhost:1503":
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/trentm/tm/opentelemetry-js4/experimental/packages/exporter-logs-otlp-grpc/test/OTLPLogExporter.test.ts)
      at listOnTimeout (node:internal/timers:594:17)
      at processTimers (node:internal/timers:529:7)

  3) OTLPLogExporter - node without TLS, with metadata, target https://localhost:1503
       "before all" hook in "OTLPLogExporter - node without TLS, with metadata, target https://localhost:1503":
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/trentm/tm/opentelemetry-js4/experimental/packages/exporter-logs-otlp-grpc/test/OTLPLogExporter.test.ts)
      at listOnTimeout (node:internal/timers:594:17)
      at processTimers (node:internal/timers:529:7)

  4) OTLPLogExporter - node without TLS, without metadata, target unix:///tmp/otlp-logs.sock
       "before all" hook in "OTLPLogExporter - node without TLS, without metadata, target unix:///tmp/otlp-logs.sock":
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/trentm/tm/opentelemetry-js4/experimental/packages/exporter-logs-otlp-grpc/test/OTLPLogExporter.test.ts)
      at listOnTimeout (node:internal/timers:594:17)
      at processTimers (node:internal/timers:529:7)

After:

...
  1) OTLPLogExporter - node with TLS, without metadata, target https://localhost:1503
       "before all" hook for "test certs are valid":
     RangeError: "length" is outside of buffer bounds
      at Buffer.proto.utf8Write (node:internal/buffer:1066:13)
      at Op.writeStringBuffer [as fn] (/Users/trentm/tm/opentelemetry-js4/node_modules/protobufjs/src/writer_buffer.js:61:13)
      at BufferWriter.finish (/Users/trentm/tm/opentelemetry-js4/node_modules/protobufjs/src/writer.js:453:14)
      at /Users/trentm/tm/opentelemetry-js4/node_modules/@grpc/proto-loader/src/index.ts:382:62
      at Array.map (<anonymous>)
      at createPackageDefinition (/Users/trentm/tm/opentelemetry-js4/node_modules/@grpc/proto-loader/src/index.ts:381:47)
      at /Users/trentm/tm/opentelemetry-js4/node_modules/@grpc/proto-loader/src/index.ts:434:12

  2) OTLPLogExporter - node without TLS, without metadata, target https://localhost:1503
       "before all" hook in "OTLPLogExporter - node without TLS, without metadata, target https://localhost:1503":
     RangeError: "length" is outside of buffer bounds
      at Buffer.proto.utf8Write (node:internal/buffer:1066:13)
      at Op.writeStringBuffer [as fn] (/Users/trentm/tm/opentelemetry-js4/node_modules/protobufjs/src/writer_buffer.js:61:13)
...

I.e. the underlying error is no longer masked.

@trentm trentm self-assigned this Aug 23, 2024
@trentm trentm requested a review from a team August 23, 2024 23:01
@trentm
Copy link
Contributor Author

trentm commented Aug 23, 2024

This could likely also be done for the other exporter tests, assuming they have the same boilerplate setting up some of the test cases.

@waitingsong
Copy link

cause of protobufjs : protobufjs/protobuf.js#2025

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks 👍

@pichlermarc
Copy link
Member

This could likely also be done for the other exporter tests, assuming they have the same boilerplate setting up some of the test cases.

Yes, I'm going though quite a few of them during the exporter rewrite and I'm switching them over one by one. There are quite a few of these instances spread all across the repo (not only the exporters).

@pichlermarc pichlermarc added this pull request to the merge queue Aug 26, 2024
Merged via the queue into open-telemetry:main with commit 2ca2459 Aug 26, 2024
18 of 20 checks passed
trentm added a commit to trentm/opentelemetry-js that referenced this pull request Sep 4, 2024
Now that Node.js 22.8 is available with a workaround
(nodejs/node#54565) for the bug in 22.7 that
caused 'RangeError: "length" is outside of buffer bounds' errors in
exporter tests (see open-telemetry#4953),
we can unpin the Node.js v22 used for unit tests.
This undoes the pinning from open-telemetry#4957.
Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants