feat: Puma plugin to gracefully shut down providers

[bdewater-thatch]

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!

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: bdewater-thatch / name: Bart de Water (0d001f2, db27403, aa91b90, 727d33b, c8a7f5b)

[kaylareopelle]

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

[kaylareopelle]

Love the inclusion of metrics and logs! 🎉

[bdewater-thatch]

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.

[kaylareopelle]

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? 🤔

[scbjans]

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.

[robbkidd]

🤔 Could try() help here maybe with a warning along the lines of "do you have an OTel SDK loaded?" logged if there isn't a shutdown() method implemented on the configured tracer_provider?

[bdewater-thatch]

I'm not following the "shutdown method in the API" discussion. I've looked at what Sidekiq does (which is seemingly the only example of instrumentation calling shutdown) and it doesn't do anything additional checks?

opentelemetry-ruby-contrib/instrumentation/sidekiq/lib/opentelemetry/instrumentation/sidekiq/instrumentation.rb

Lines 134 to 136 in 2edff11

	config.on(:shutdown) do
	OpenTelemetry.tracer_provider.shutdown
	end

[bdewater-thatch]

I found this article to help me understand the difference between API and SDK. I've added an early return based on this example - since that already logs a warning I don't think we need another log here?

[bdewater-thatch]

@kaylareopelle I addressed your feedback. Please LMK if you prefer additional commits for ease of review or for me to squash.

[xuan-cao-swi]

The main thing that stands out to me, is that this instrumentation isn't emitting any telemetry.

I have the same concern. The plugin could be in a different folder (e.g. /plugins, similar to resource detector), and then user can install them independently and use it if they need.