Rewrite emailservice in ruby #109
Conversation
Rather than use GRPC, we reimplement the emailservice as a simple HTTP application, using Sinatra and OpenTelemetry auto-instrumentation. We can do this because our use of GRPC in this instance is not very complicated - we can just serialize the protobuf request to JSON and send it over HTTP instead, and it's pretty straightforward. Probably the most interesting part here is that we're rendering the email text right into the logfile, which might be noisy. We can revisit that decision if needed.
This commit causes the checkoutservice to call emailservice via HTTP POST. We leverage the golang net/http autoinstrumentation to create the spans and propagate context correctly for us.
Because we rewrote the emailservice as an HTTP service, we need to modify the `EMAIL_SERVICE_ADDR` to reflect the actual scheme (`http`). We could just do some more string concatenation in the app, I guess. We pin to a specific collector version - I was getting weird segfaults with whatever version of the collector was already on my machine, and they were resolved with 0.52.0. However, that also required a minor config change to the jaeger exporter. Yay, M1 macs! We can't specify the otlp/grpc port for the ruby libraries, it won't work. So for email service we pass the correct OTLP/HTTP URL in the env var.
|
Obligatory screenshot!
|
Whoops, this was accidentally left in from my testing.
|
Hello @ahayworth, why is gRPC not best friends with Ruby? The PR looks good to me, but before approving this PR I'd like to hear the opinion from @cartersocha and @austinlparker. I personally do not care if it is gRPC or HTTP, but as far as I remember we would keep gRPC wherever possible in the sample. |
Lack of limitations does not imply that it works well, sadly. I wish they were better friends. Technical things:
Socio-technical things:
OpenTelemetry-specific things:
No small amount of these issues are related / intertwined with
All of this aside: it's definitely possible to use GRPC with Ruby. But it's generally a poor experience, and I've personally been annoyed every time I try to do it. There are a lot of outstanding technical issues with both GRPC and protobuf, and the ruby wrappers don't see a lot of love from the maintainers. There are frequently issues with new versions, and my sense is that "nobody on these projects is testing these releases with Ruby" - just the sheer number of "we can't install the new version on ruby" issues alone demonstrates as much. Setting that aside, using the grpc gem is just confusing and not ruby-like. Trying to dive in the code is a huge mess, from my experience. It's not impossible, but it feels like a half-baked project for Ruby in a lot of ways. Of course, that's a subjective feeling on my part but I think I've linked enough issues and discussions to at least show I'm not alone. 馃槅 I really don't know why this is, I guess. My own personal thought is that it's a bad idea to just implement client libraries as thin wrappers around C extensions without really trying to implement things in a language-first fashion. Anyways: is it impossible to do this as a GRPC service? No, but I'd really prefer not to. Setting aside the general annoyances, it's just not a good demo for Ruby. The community is heavily entrenched in HTTP, and that's what is shown off the best. We don't even have GRPC instrumentation - and precious few people have even asked. So even if we did this in GRPC, it's just not going to be as nice as an HTTP version. |
|
Awesome @ahayworth! Discussed with everyone at the SIG meeting and this is good to go! 馃殌 |
If I had invested, say, a mere modicum of effort into understanding what needed to be done to use a `-slim` image in the first place, then we would have included this in #109 from the start. However, I clearly did not. 馃う This change switches us to the `-slim` image, and installs the one thing we actually need to build `puma`: Debian's `build-essential` package.
| require "opentelemetry/instrumentation/sinatra" | ||
|
|
||
| OpenTelemetry::SDK.configure do |c| | ||
| c.use "OpenTelemetry::Instrumentation::Sinatra" |
There was a problem hiding this comment.
I think c.use_all is the use that we want to steer folks towards.
| end | ||
|
|
||
| post "/send_order_confirmation" do | ||
| data = JSON.parse(request.body.read, object_class: OpenStruct) |
There was a problem hiding this comment.
Any change we can use a Hash, Struct or Object here instead to mitigate memory leaks?
There was a problem hiding this comment.
Feel free to open a pr with the proposed changes. I know @ahayworth would love the help!
There was a problem hiding this comment.
Any change we can use a Hash, Struct or Object here instead to mitigate memory leaks?
This was the fastest path to victory without mucking up the already existing email template. But, in general: yes, we absolutely could!
|
|
||
| require "opentelemetry/sdk" | ||
| require "opentelemetry/exporter/otlp" | ||
| require "opentelemetry/instrumentation/sinatra" |
There was a problem hiding this comment.
Can we use Bundler here to load dependencies and use Bundler.require instead of requiring each dependency separately?
There was a problem hiding this comment.
I believe so, yes.
* feat: rewrite emailservice in ruby Rather than use GRPC, we reimplement the emailservice as a simple HTTP application, using Sinatra and OpenTelemetry auto-instrumentation. We can do this because our use of GRPC in this instance is not very complicated - we can just serialize the protobuf request to JSON and send it over HTTP instead, and it's pretty straightforward. Probably the most interesting part here is that we're rendering the email text right into the logfile, which might be noisy. We can revisit that decision if needed. * feat: call emailservice via HTTP POST This commit causes the checkoutservice to call emailservice via HTTP POST. We leverage the golang net/http autoinstrumentation to create the spans and propagate context correctly for us. * feat: modify setup to work with otel/ruby Because we rewrote the emailservice as an HTTP service, we need to modify the `EMAIL_SERVICE_ADDR` to reflect the actual scheme (`http`). We could just do some more string concatenation in the app, I guess. We pin to a specific collector version - I was getting weird segfaults with whatever version of the collector was already on my machine, and they were resolved with 0.52.0. However, that also required a minor config change to the jaeger exporter. Yay, M1 macs! We can't specify the otlp/grpc port for the ruby libraries, it won't work. So for email service we pass the correct OTLP/HTTP URL in the env var. * docs: note that emailservice is now ruby * fixup: remove `loglevel: debug` from otel-col config Whoops, this was accidentally left in from my testing. * fixup: appease markdownlint
If I had invested, say, a mere modicum of effort into understanding what needed to be done to use a `-slim` image in the first place, then we would have included this in open-telemetry#109 from the start. However, I clearly did not. 馃う This change switches us to the `-slim` image, and installs the one thing we actually need to build `puma`: Debian's `build-essential` package.
* feat: rewrite emailservice in ruby Rather than use GRPC, we reimplement the emailservice as a simple HTTP application, using Sinatra and OpenTelemetry auto-instrumentation. We can do this because our use of GRPC in this instance is not very complicated - we can just serialize the protobuf request to JSON and send it over HTTP instead, and it's pretty straightforward. Probably the most interesting part here is that we're rendering the email text right into the logfile, which might be noisy. We can revisit that decision if needed. * feat: call emailservice via HTTP POST This commit causes the checkoutservice to call emailservice via HTTP POST. We leverage the golang net/http autoinstrumentation to create the spans and propagate context correctly for us. * feat: modify setup to work with otel/ruby Because we rewrote the emailservice as an HTTP service, we need to modify the `EMAIL_SERVICE_ADDR` to reflect the actual scheme (`http`). We could just do some more string concatenation in the app, I guess. We pin to a specific collector version - I was getting weird segfaults with whatever version of the collector was already on my machine, and they were resolved with 0.52.0. However, that also required a minor config change to the jaeger exporter. Yay, M1 macs! We can't specify the otlp/grpc port for the ruby libraries, it won't work. So for email service we pass the correct OTLP/HTTP URL in the env var. * docs: note that emailservice is now ruby * fixup: remove `loglevel: debug` from otel-col config Whoops, this was accidentally left in from my testing. * fixup: appease markdownlint
If I had invested, say, a mere modicum of effort into understanding what needed to be done to use a `-slim` image in the first place, then we would have included this in open-telemetry#109 from the start. However, I clearly did not. 馃う This change switches us to the `-slim` image, and installs the one thing we actually need to build `puma`: Debian's `build-essential` package.
Changes
This PR rewrites the
emailservicein Ruby. We make the following notable changes:checkoutserviceaccordingly.jaegerexporter section.We were able to do this pretty easily, especially since the GRPC request proto for emailservice was very simple and readily serialized to JSON.
There are two potential things wrong with it:
Fixes #37