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

fix(otlp-grpc-exporter-base): avoid TypeError on exporter shutdown #4612

Conversation

pichlermarc
Copy link
Member

Which problem is this PR solving?

Calling shutdown() on any gRPC exporter throws a TypeError - preventing apps from gracefully shutting down.

The transport should call close() instead of shutdown() on the _client. The test I wrote for this case was entirely ineffective as I also managed to mock the wrong thing.

Fixes #4611

Short description of the changes

  • call this._client?.close() instead of this._client?.shutdown()
  • adapt tests to actually test that behavior

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Updated unit tests

@pichlermarc pichlermarc marked this pull request as ready for review April 8, 2024 09:43
@pichlermarc pichlermarc requested a review from a team April 8, 2024 09:43
@pichlermarc pichlermarc added bug Something isn't working priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies, etc pkg:exporter-trace-otlp-grpc pkg:exporter-metrics-otlp-grpc pkg:exporter-logs-otlp-grpc labels Apr 8, 2024
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Looks good but I'm curious how the types allowed shutdown to be called if it doesn't exist in the first place. Probably not important for this PR, but our GRPC types might be wrong

@pichlermarc
Copy link
Member Author

pichlermarc commented Apr 11, 2024

Looks good but I'm curious how the types allowed shutdown to be called if it doesn't exist in the first place. Probably not important for this PR, but our GRPC types might be wrong

Yes, I've been using any instead of Client from the gRPC library as there are no types for the service client functions itself.
I've pushed a commit to use the Client type.

We still have to use it as any when we call export(), but at least the common Client functions are now typed.

@pichlermarc pichlermarc merged commit 3438777 into open-telemetry:main Apr 11, 2024
18 checks passed
@jdespatis
Copy link

hello @pichlermarc,

Thanks for your fix (I need it to force flushing BatchSpanProcessor when my script ends, on beforeExit event)

Can you please publish this new version ?
so that @Vunovati can update his PR Vunovati/otlp-logger#61 to release new versions.

@pichlermarc
Copy link
Member Author

hello @pichlermarc,

Thanks for your fix (I need it to force flushing BatchSpanProcessor when my script ends, on beforeExit event)

Can you please publish this new version ? so that @Vunovati can update his PR Vunovati/otlp-logger#61 to release new versions.

On it, hoping to get the release out by monday 🙂

Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
…pen-telemetry#4612)

* fix(otlp-grpc-exporter-base): avoid TypeError on exporter shutdown

* chore: update changelog

* fix: use gRPC Client type over any

* fixup! fix: use gRPC Client type over any

* fix: use ts-lint/ts-ignore
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pkg:exporter-logs-otlp-grpc pkg:exporter-metrics-otlp-grpc pkg:exporter-trace-otlp-grpc priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gracefull shutdown doesn't work with GRPC Exporter
3 participants