OpenTracing support #669

jmprusi · 2018-04-18T10:01:33Z

Adds basic opentracing jaeger support to apicast

Adds Opentracing support to apicast.
Example config for Jaeger tracer.
Uses opentracing_load_tracer to load tracing libraries dynamically.
New ENV vars
- opentracing_tracer: tracer to include, now only "jaeger" is available
- opentracing_config: config to be used by the tracer, can be configmap mounted inside the container.
- opentracing_forward_header: name of the http header used by the tracing libraries (defaults to uber-trace-id)

Requires 3scale/s2i-openresty#56, 3scale/s2i-openresty#57

update fork

mikz · 2018-04-18T11:00:20Z

gateway/conf/nginx.conf.liquid

@@ -5,12 +5,16 @@ env RESOLVER;
 env BACKEND_ENDPOINT_OVERRIDE;
 env OPENSSL_VERIFY;

+{% if jaeger_enabled %}
+load_module /opt/app-root/src/nginx-modules/ngx_http_opentracing_module.so;
+load_module /opt/app-root/src/nginx-modules/ngx_http_jaeger_module.so;


This will not work on macOS or any local machine. We have to make this relative somehow.

mikz · 2018-04-18T11:04:34Z

gateway/conf/nginx.conf.liquid

@@ -61,37 +67,38 @@ http {
    {% include file %}
  {% endfor %}

+
+  {% if jaeger_enabled %}
+    {% include "conf.d/jaeger.conf" %}


We can have a opentracing subfolder there and do:

{% include "conf.d/opentracing" | append opentracing_tracer | append ".conf" %}

So we have a conf file for each supported tracer.

mikz · 2018-04-18T11:05:22Z

gateway/conf/nginx.conf.liquid

 daemon {{ daemon | default: 'off' }};
 master_process {{ master_process | default: 'on' }};
 worker_processes {{ worker_processes | default: 'auto' }};
 pcre_jit on;
 pid {{ pid | default: 'nginx.pid' }};
-timer_resolution {{ timer_resolution | default: '100ms' }};


This should be disabled only when opentracing is enabled. Or the timer_resolution should be set to 0 (if that works).

mikz · 2018-04-18T11:22:51Z

gateway/conf.d/echo.conf


-    echo_read_request_body;
+location / {


We probably don't need to rewrite this right? The echo module behaves ok.

mikz · 2018-04-18T11:30:16Z

gateway/conf.d/backend.conf

@@ -1,4 +1,11 @@
+{% if jaeger_enabled %}


No liquid in this template (it would need to have .liquid extension). This will neeed to be in the main nginx.conf.liquid.

mikz · 2018-04-18T11:36:20Z

gateway/conf.d/apicast.conf

@@ -9,6 +11,12 @@ set_by_lua_block $deployment {
 # ssl_session_fetch_by_lua_block { require('apicast.executor').call() }
 # ssl_session_store_by_lua_block { require('apicast.executor').call() }

+{% if jaeger_enabled %}


This is trouble. There is no liquid templating in this file. This global definition can be in the nginx.conf.liquid.

mikz · 2018-04-18T11:36:53Z

gateway/conf.d/apicast.conf

  proxy_pass_request_headers off;

+  {% if jaeger_enabled %}


This is impossible to do. This file has to stay plain nginx file without liquid templating because of our test suite.

mikz · 2018-04-18T12:35:38Z

gateway/conf.d/jaeger.conf

@@ -0,0 +1,4 @@
+jaeger_reporter_local_agent_host_port {{ jaeger_addr | default: '127.0.0.1' }}:{{ jaeger_port | default: 6831 }};


We should add .liquid extension to files that are processed by liquid so it is more obvious liquid is available there.

mikz · 2018-04-18T12:35:43Z

gateway/src/apicast/cli/environment.lua

@@ -108,7 +108,10 @@ _M.default_config = {
    proxy_ssl_certificate_key = env_value_ref('APICAST_PROXY_HTTPS_CERTIFICATE_KEY'),
    proxy_ssl_session_reuse = env_value_ref('APICAST_PROXY_HTTPS_SESSION_REUSE'),
    proxy_ssl_password_file = env_value_ref('APICAST_PROXY_HTTPS_PASSWORD_FILE'),
-
+    jaeger_enabled = env_value_ref('JAEGER_ENABLED'),


I'd rather try to make this "opentracing" support rather than just jaeger.

IMO we should be open to support more than jaeger tracker and that the configuration should be designed like that.

So for example jaeger_enabled IMO should be opentracing_enabled and then there should be opentracing_tracer that says jaeger|...|...|....

The common denominator of the configuration should be defined as "opentracing" and then the specifics can be named with the tracer name.

jmprusi · 2018-04-20T10:50:10Z

gateway/conf.d/apicast.conf

@@ -35,6 +35,7 @@ location = /___http_call {
  proxy_set_header User-Agent $user_agent;
  proxy_set_header X-3scale-OAuth2-Grant-Type $grant_type;
  proxy_set_header 3scale-options $options;
+  proxy_set_header uber-trace-id $http_uber_trace_id;


@mikz @davidor we cannot template this apicast file isn't it?

This header in jaeger is user customizable, so, we should allow some way to change this via ENV, any idea? some include?

mikz · 2018-04-26T13:38:42Z

gateway/conf/nginx.conf.liquid

 daemon {{ daemon | default: 'off' }};
 master_process {{ master_process | default: 'on' }};
 worker_processes {{ worker_processes | default: 'auto' }};
 pcre_jit on;
 pid {{ pid | default: 'nginx.pid' }};
+
+{% unless opentracing_tracer != empty %}


I think this should be if.

mikz · 2018-04-26T15:13:54Z

CHANGELOG.md

@@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 ### Added

 - Rate Limit policy [PR #648](https://github.com/3scale/apicast/pull/648)
+- Opentracing support [PR #669](https://github.com/3scale/apicast/pull/669)


👀 OpenTracing?

jmprusi · 2018-04-27T14:26:32Z

t/opentracing.t

+--- error_code: 200
+--- no_error_log
+[error]
+--- udp_listen: 6831


That's openresty? cool.

Thats the Test::Nginx testing framework: http://search.cpan.org/~agent/Test-Nginx/lib/Test/Nginx/Socket.pm#udp_listen

…he server part in nginx.conf.liquid

includes 3scale/s2i-openresty#57

mikz

👍 sweet

We got OpenTracing support on Linux and macOS.
Homebrew formulas provided in our tap: 3scale-archive/homebrew-opentracing#1

Merge pull request #1 from 3scale/master

b64a77f

update fork

octobot assigned jmprusi Apr 18, 2018

jmprusi requested review from mikz and davidor April 18, 2018 10:43

mikz reviewed Apr 18, 2018

View reviewed changes

jmprusi force-pushed the opentracing branch 4 times, most recently from c458cf6 to b3f904a Compare April 19, 2018 16:24

jmprusi commented Apr 20, 2018

View reviewed changes

jmprusi changed the title ~~WIP Opentracing~~ Opentracing support Apr 20, 2018

jmprusi force-pushed the opentracing branch 4 times, most recently from e10da83 to f1090fd Compare April 20, 2018 18:17

mikz reviewed Apr 26, 2018

View reviewed changes

jmprusi force-pushed the opentracing branch from 80a0308 to b9c41fb Compare April 26, 2018 15:18

jmprusi changed the title ~~Opentracing support~~ OpenTracing support Apr 26, 2018

jmprusi commented Apr 27, 2018

View reviewed changes

jmprusi added 2 commits April 27, 2018 16:52

Merge remote-tracking branch 'upstream/master'

a945c2f

Adds jaeger config

163d2b0

jmprusi and others added 20 commits April 27, 2018 16:58

Include opentracing modules and jaeger conf

1a1d3aa

Sets proper service_name and location_names for all servers

9dde2b9

Read jaeger vars from ENV

925aaf4

created opentracing directory to include individual tracer confs

b1466d1

Adds new ENV vars for opentracing_enabled and opentracing_tracer

4d38495

Remove the specific configs from each conf and put all of them into t…

fb9ded4

…he server part in nginx.conf.liquid

Checkout from master

63a58d7

Replace endif to endunless

f27a9d2

Use opentracing_tracer instead of a bool

cfb6950

Forwared uber-trace-id header

49c1898

Use opentracing load tracer for jaeger

fa38640

Allow the user to change the config file path

1950a75

Use uber-trace-id by default

597f370

Allow to set the opentracing forward header via ENV

c5febb5

CHANGELOG: OpenTracing support

493145b

Fix CHANGELOG merge

f949669

[nginx] search for modules in standard nginx directory

4225d52

[opentracing] ability to load jaegertracing on macOS

e8377c1

[compose] provide jaeger all-in-one to try opentracing

fdbe642

[t] test opentracing

6a0dbc0

jmprusi force-pushed the opentracing branch from 55e05da to 6a0dbc0 Compare April 27, 2018 14:58

mikz force-pushed the opentracing branch from 5988895 to f961223 Compare April 30, 2018 16:12

[circle] bump s2i-openresty to 1.13.6.1-rover12

6e51fcd

includes 3scale/s2i-openresty#57

mikz force-pushed the opentracing branch from f961223 to 6e51fcd Compare April 30, 2018 16:22

mikz approved these changes Apr 30, 2018

View reviewed changes

mikz merged commit 78aa7ab into master Apr 30, 2018

mikz deleted the opentracing branch April 30, 2018 17:25

davidor added this to the 3.3 milestone Jun 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenTracing support #669

OpenTracing support #669

jmprusi commented Apr 18, 2018 •

edited by mikz

Loading

mikz Apr 18, 2018

mikz Apr 18, 2018

mikz Apr 18, 2018

mikz Apr 18, 2018

mikz Apr 18, 2018

mikz Apr 18, 2018

mikz Apr 18, 2018

mikz Apr 18, 2018

mikz Apr 18, 2018

jmprusi Apr 20, 2018

mikz Apr 26, 2018

mikz Apr 26, 2018

jmprusi Apr 26, 2018

jmprusi Apr 27, 2018

mikz Apr 27, 2018

mikz left a comment

		@@ -0,0 +1,4 @@
		jaeger_reporter_local_agent_host_port {{ jaeger_addr \| default: '127.0.0.1' }}:{{ jaeger_port \| default: 6831 }};

OpenTracing support #669

OpenTracing support #669

Conversation

jmprusi commented Apr 18, 2018 • edited by mikz Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikz left a comment

Choose a reason for hiding this comment

jmprusi commented Apr 18, 2018 •

edited by mikz

Loading