feat: Puma plugin to gracefully shut down providers #1637
Conversation
bdewater-thatch
requested review from
fbogsany,
mwear,
robertlaurin,
dazuma,
ericmustin,
arielvalentin,
ahayworth,
plantfansam,
robbkidd,
simi,
kaylareopelle,
xuan-cao-swi and
a team
as code owners
The committers listed above are authorized under a signed CLA. |
There was a problem hiding this comment.
Thank you for your contribution, @bdewater-thatch! 😄 I took a first pass on this PR and left a few comments related to the repo's best practices. Overall, this looks good! I still want to do more testing and thinking before I approve.
The main thing that stands out to me, is that this instrumentation isn't emitting any telemetry. (I apologize, we started talking Puma stats in Slack ages ago and I lost track of things 😅).
I'm not sure if that's a problem for the initial release. Perhaps calling out what the gem does a little more in the README is enough for now, since the shutdown problem already plagues users. What do you think?
cc'ing the folks in the on_shutdown slack thread: @arielvalentin, @hibachrach, @wsmoak
| ## TODO: Include the supported version to be tested here. | ||
| ## Example: | ||
| # appraise 'rack-2.1' do | ||
| # gem 'rack', '~> 2.1.2' | ||
| # end | ||
|
|
||
| # appraise 'rack-2.0' do | ||
| # gem 'rack', '2.0.8' | ||
| # end |
There was a problem hiding this comment.
Calling out this TODO! 🙂 We usually like to test the latest version of the gem and at least one major version pinned. If those end up being the same version, feel free to just use the latest.
| # DO NOT ADD DEPENDENCIES HERE! | ||
| # Please declare a minimum development dependency in the gemspec, | ||
| # then target specific versions in the Appraisals file. | ||
|
|
There was a problem hiding this comment.
Whoops, this isn't accurate anymore. I updated the generator in #1641. Let's remove it.
| # DO NOT ADD DEPENDENCIES HERE! | |
| # Please declare a minimum development dependency in the gemspec, | |
| # then target specific versions in the Appraisals file. |
|
|
||
| spec.add_dependency 'opentelemetry-api', '~> 1.4.0' | ||
| spec.add_dependency 'opentelemetry-instrumentation-base', '~> 0.23.0' | ||
| spec.add_dependency 'puma', '~> 6.0' |
There was a problem hiding this comment.
We don't like to require the gem we're instrumenting in the Gemspec. This interferes with dependency detection when using opentelemetry-instrumentation-all. If you'd like to set a minimum version for Puma, the puma/instrumentation.rb file is a better place!
We use the compatible method to check versions. This file is a good example of how to use it.
| spec.add_dependency 'puma', '~> 6.0' |
| OpenTelemetry.tracer_provider.shutdown | ||
| OpenTelemetry.meter_provider.shutdown if OpenTelemetry.respond_to?(:meter_provider) | ||
| OpenTelemetry.logger_provider.shutdown if OpenTelemetry.respond_to?(:logger_provider) |
There was a problem hiding this comment.
Love the inclusion of metrics and logs! 🎉
There was a problem hiding this comment.
It's a bit forward looking but figured it wouldn't hurt :)
I forgot to ask but was wondering if you think this should include a configurable timeout option, and if so what the default should be (nil?). We host our apps on Render, which has a 30s timeout by default for graceful shutdown but is configurable via API/IaC to be up to 300s.
There was a problem hiding this comment.
Ooh, that's a good question. I'm not sure. Thinking out loud here:
Timeouts could technically be set through OTEL_BSP_EXPORT_TIMEOUT since the tracer provider will iterate through all the span processors on shutdown and they'll use that setting. However, the tracer provider's shutdown method has its own timeout that wraps the processors, which would have a different value.
The API doesn't have a shutdown method for the TracerProvider, and one isn't mentioned in the spec. The SDK defines the shutdown method. Instrumentation should be implementation-agnostic, so we don't include the SDK as a dependency in instrumentation. I wonder if we need to add a no-op shutdown API to allow this to work? And if that's even spec compliant? 🤔
There was a problem hiding this comment.
I'd like to add that there is indeed no shutdown method in the API; therefore we need to guard for when someone uses the OTEL_SDK_DISABLED envvar.
Following discussion at open-telemetry/opentelemetry-ruby#1357 and on Slack I decided to take a moment and write a Puma plugin to shutdown tracer/meter/logger providers and make sure nothing is lost before the process exits. Given that puma is the default for newly generated Rails apps, this should make things "just work" a little better.
First time contributor, I'm sure I'm missing things :) any feedback welcome!