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

THREESCALE-9682: Add support for OpenTelemetry #405

Merged
merged 5 commits into from
Sep 16, 2024

Conversation

jlledom
Copy link
Contributor

@jlledom jlledom commented Sep 5, 2024

This adds OpenTelemetry instrumentation for Sinatra in Apisonator.

How to verify:

  1. Follow the instructions on the docs to launch a Jaeger container.
  2. Set OTEL_EXPORTER_OTLP_ENDPOINT="http://localhost:4318" and the new var CONFIG_OPENTELEMETRY_ENABLED=true.
  3. Work normally, e.g. send some transaction requests to the listener.
  4. Visit Jaeger UI at http://localhost:16686 and verify the requests are reported.
  5. Verify that not setting CONFIG_OPENTELEMETRY_ENABLED or setting it to false disables the metrics and you can't see the requests in Jaeger.

Jira Issue:
https://issues.redhat.com/browse/THREESCALE-9682

@jlledom jlledom self-assigned this Sep 5, 2024
@akostadinov
Copy link
Contributor

Is it possible by any chance to test that the endpoint is operational with the said env variable? Maybe not, just asking.

@jlledom
Copy link
Contributor Author

jlledom commented Sep 6, 2024

Is it possible by any chance to test that the endpoint is operational with the said env variable? Maybe not, just asking.

I don't think there's a way other than pinging the endpoint host and port directly.

@akostadinov
Copy link
Contributor

Fair enough. But how do we know whether this fixes the actual JIRA issue THREESCALE-9669?

@jlledom
Copy link
Contributor Author

jlledom commented Sep 6, 2024

Fair enough. But how do we know whether this fixes the actual JIRA issue THREESCALE-9669?

The issue is not only about apisonator. As I understood it, the only apisonator work required is:

3scale Backend should be instrumented using opentelemetry SDK. Sinatra has instrumentation in place implemented in https://github.com/open-telemetry/opentelemetry-ruby-contrib/tree/main/instrumentation/sinatra

So there's no need to check any endpoint AFAIK. @eguzki ?

@eguzki
Copy link
Member

eguzki commented Sep 12, 2024

That's the one jira issue describing the work being done here: https://issues.redhat.com/browse/THREESCALE-9682

@eguzki eguzki changed the title THREESCALE-9669: Add support for OpenTelemetry THREESCALE-9682: Add support for OpenTelemetry Sep 12, 2024
@eguzki
Copy link
Member

eguzki commented Sep 12, 2024

LGTM

I left one suggestion, but it's a nit.

@eguzki
Copy link
Member

eguzki commented Sep 12, 2024

It would be nice to drop an snapshot from jaeger UI with the trace being recorded from backend listener

@@ -53,6 +53,7 @@
require '3scale/backend/failed_jobs_scheduler'
require '3scale/backend/transactor'
require '3scale/backend/listener'
require '3scale/backend/opentelemetry'
Copy link
Member

Choose a reason for hiding this comment

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

This is only executed from the backend listener?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd say it only makes sense for the listener, which is who listens to sinatra routes

Copy link
Member

Choose a reason for hiding this comment

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

the worker is also importing this file.

What about lib/3scale/backend/server.rb ?? I did not try myself, though. Looks like lib/3scale/backend/listener.rb is being imported by both worker and listener 🤷

Not important, though.. up to you.. not a request for change. The worker will not have the opt-in switch enabled, so it is harmless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed: a45900b

Co-authored-by: Eguzki Astiz Lezaun <[email protected]>
@jlledom
Copy link
Contributor Author

jlledom commented Sep 13, 2024

It would be nice to drop an snapshot from jaeger UI with the trace being recorded from backend listener

Some screenshots:

image
image
image
image

@jlledom jlledom merged commit 58baa5c into 3scale:master Sep 16, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants