Skip to content

Conversation

@iilyak
Copy link
Contributor

@iilyak iilyak commented Jan 27, 2020

Overview

This PR adds support for http reporter for open tracing library. The http reporter is useful in following two cases:

  • when you deployment doesn't include jaeger agents and you want to communicate directly to jaeger server
  • when spans are greater than max udp packet size (65Kb).

Testing recommendations

  1. update rel/overlay/etc/default.ini as follows
diff --git a/rel/overlay/etc/default.ini b/rel/overlay/etc/default.ini
index 592445c28..69091eab3 100644
--- a/rel/overlay/etc/default.ini
+++ b/rel/overlay/etc/default.ini
@@ -572,15 +572,15 @@ min_priority = 2.0
 ;   - jaeger.thrift over udp
 ;   - jaeger.thrift over http
 ; ## Common settings
-; enabled = false ; true | false
-; app_name = couchdb ; value to use for the `location.application` tag
-; protocol = udp ; udp | http - which reporter to use
+enabled = true ; true | false
+app_name = couchdb ; value to use for the `location.application` tag
+protocol = http ; udp | http - which reporter to use
 ; ## jaeger.thrift over udp reporter
 ; thrift_format = compact ; compact | binary
 ; agent_host = 127.0.0.1
 ; agent_port = 6831
 ; ## jaeger.thrift over udp reporter
-; endpoint = http://127.0.0.1:14268
+endpoint = http://127.0.0.1:14268
 [tracing.filters]
 ;
@@ -603,4 +603,4 @@ min_priority = 2.0
 ; corresponding operation name key configured. Thus, users can easily
 ; log every generated trace by including the following:
 ;
-; all = (#{}) -> true
+all = (#{}) -> true
  1. start jaeger in docker container
docker run -d -p 6831:6831/udp -p 16686:16686 jaegertracing/all-in-one:1.14
  1. Start couchdb dev/run --admin=adm:pass
  2. Create database curl -X PUT -u adm:pass http://127.0.0.1:15984/test
  3. Query database curl -X GET -u adm:pass http://127.0.0.1:15984/test -H 'X-B3-TraceId: 12345678901234567890123453782027'
  4. Open http://localhost:16686/ in the browser and search for spans (you should find some traces)

Related Issues or Pull Requests

This PR depends on

Checklist

  • Code is written and works correctly
  • Changes are covered by tests
  • Any new configurable parameters are documented in rel/overlay/etc/default.ini
  • A PR for documentation changes has been made in https://github.com/apache/couchdb-documentation

{hyper, "hyper", {tag, "CouchDB-2.2.0-4"}},
{ibrowse, "ibrowse", {tag, "CouchDB-4.0.1-1"}},
{jaeger_passage, "jaeger-passage", {tag, "CouchDB-0.1.13-1"}},
{jaeger_passage, "jaeger-passage", "7f56a93393070fb6e3778615a2d040573b10a6f3"},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This sha would be updated to {tag, "CouchDB-0.1.14-1"} when apache/couchdb-jaeger-passage#1 is merged and correspondent tag created.

Copy link
Member

Choose a reason for hiding this comment

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

Please merge that first, don't put shas into master.

Copy link
Contributor

@chewbranca chewbranca left a comment

Choose a reason for hiding this comment

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

Minor nit on asserting ok, but other than that looks good to me!

{protocol, udp},
{default_service_name, ServiceName}
],
jaeger_passage:start_tracer(TracerId, Sampler, Options);
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit, but the previous version of this function matched ok against jaeger_passage:start_tracer(...), whereas this version does not. Was that an intentional behavior change?

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 in a5445d7

Copy link
Contributor

@jaydoane jaydoane left a comment

Choose a reason for hiding this comment

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

LGTM, although I highly recommend making that DRY change. If not, I'm happy to open a PR for it myself. Attached is a screenshot of my local jaeger after the test run:
Screen Shot 2020-01-27 at 7 46 55 PM

There is a global toggle `[tracing] enabled = true | false` that switches
tracing on or off completely. The `[tracing]` section also includes
configuration for where to send trace data.
configuration for where to send trace data. There are two reporter which we
Copy link
Contributor

Choose a reason for hiding this comment

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

s/reporter/reporters/

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 by f825540

passage_sampler_all:new(), TracerId, MaxQueueLen),
ServiceName = list_to_atom(config:get("tracing", "app_name", "couchdb")),

case config:get("tracing", "protocol", "udp") of
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer a DRYer approach here, where the tracer is only started in one place:

    Options = [
        {default_service_name, ServiceName}
    ] ++ case config:get("tracing", "protocol", "udp") of
        "udp" ->
            [
                {protocol, udp},
                {thrift_format, list_to_atom(
                    config:get("tracing", "thrift_format", "compact"))},
                {agent_host,
                    config:get("tracing", "agent_host", "127.0.0.1")},
                {agent_port,
                    config:get_integer("tracing", "agent_port", 6831)}
            ];
        "http" ++ _ ->
            [
                {protocol, http},
                {endpoint,
                    config:get("tracing", "endpoint", "http://127.0.0.1:14268")},
                {http_client, fun http_client/5}
            ]
    end,
    ok = jaeger_passage:start_tracer(TracerId, Sampler, Options).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DRYed in a5445d7

@iilyak iilyak force-pushed the add-http-reporter branch from a5445d7 to bd3c021 Compare January 30, 2020 14:30
@iilyak iilyak merged commit 7e0c5bb into apache:prototype/fdb-layer Jan 30, 2020
@iilyak iilyak deleted the add-http-reporter branch January 30, 2020 14:33
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.

4 participants