Make Metrics consistent with Go client. #129

eundoosong · 2018-02-24T17:38:25Z

Make Metrics consistent with Go client.

Now the metric name shows consistent with go-client as you see below prometheus test I did.

in sampler, add SamplerMetrics which has 4 types of metrics, as go-client has.
in tracer, add spans_finished

Resolves #102

Please review.

# HELP jaeger:traces jaeger:traces
# TYPE jaeger:traces counter
jaeger:traces{sampled="n",state="started"} 6.0
jaeger:traces{sampled="y",state="started"} 0.0
jaeger:traces{sampled="y",state="joined"} 0.0
jaeger:traces{sampled="n",state="joined"} 0.0
# HELP jaeger:sampler_updates jaeger:sampler_updates
# TYPE jaeger:sampler_updates counter
jaeger:sampler_updates{result="err"} 0.0
jaeger:sampler_updates{result="ok"} 0.0
# HELP python_info Python platform information
# TYPE python_info gauge
python_info{implementation="CPython",major="2",minor="7",patchlevel="10",version="2.7.10"} 1.0
# HELP jaeger:sampler_queries jaeger:sampler_queries
# TYPE jaeger:sampler_queries counter
jaeger:sampler_queries{result="err"} 0.0
jaeger:sampler_queries{result="ok"} 0.0
# HELP jaeger:started_spans jaeger:started_spans
# TYPE jaeger:started_spans counter
jaeger:started_spans{sampled="n"} 12.0
jaeger:started_spans{sampled="y"} 0.0
# HELP jaeger:finished_spans jaeger:finished_spans
# TYPE jaeger:finished_spans counter
jaeger:finished_spans 0.0
# HELP jaeger:reporter_spans jaeger:reporter_spans
# TYPE jaeger:reporter_spans counter
jaeger:reporter_spans{result="dropped"} 0.0
jaeger:reporter_spans{result="socket_error"} 0.0
jaeger:reporter_spans{result="err"} 0.0
jaeger:reporter_spans{result="ok"} 0.0

Signed-off-by: eundoo-song eundoo.song@gmail.com

Signed-off-by: eundoo-song <eundoo.song@samsung.com>

Signed-off-by: Eundoo Song <eundoo.song@gmail.com>

coveralls · 2018-02-24T17:44:29Z

Coverage decreased (-0.008%) to 93.903% when pulling 3002240 on eundoosong:consistent_metrics into 9cbf95b on jaegertracing:master.

eundoosong · 2018-02-24T18:00:22Z

It looks like the uncovered line(return PeriodicCallback) is not related to this.
Anyone knows how to fix this?

yurishkuro · 2018-02-25T06:36:10Z

@eundoosong Sweet! Thanks for contributing. Don't worry about the coverage, it looks like a glitch.

@jpkrohling could you take a look, since you just did the same in Java?

Signed-off-by: Eundoo Song <eundoo.song@gmail.com>

jpkrohling · 2018-02-26T13:34:17Z

LGTM. Looks like the Python version is missing a gauge (reporter_queue_length) and has an extra counter (jaeger:reporter_spans{'result': 'socket_error}), but I think those differences are OK.

eundoosong · 2018-02-27T13:42:36Z

@jpkrohling Thanks for your review.
I think we can add reporter_queue_length gauge just after span batch submission like this.
Tornado provides an api to know current size of queue.
http://www.tornadoweb.org/en/stable/_modules/tornado/queues.html#Queue.qsize

if spans:
    yield self._submit(spans)
    self.metrics.reporter_queue_length(self.queue.qsize())
    for _ in spans:
        self.queue.task_done()
    spans = spans[:0]

If it's ok, I'll update.

jpkrohling · 2018-02-27T13:46:48Z

I'm not familiar with Tornado (or Python in general, for that matter), but looks OK to me.

Signed-off-by: Eundoo Song <eundoo.song@gmail.com>

eundoosong · 2018-02-27T16:42:20Z

@yurishkuro Is there any reason reporter_queue_length gauge is missed in python? Do you think it should be added as above?

yurishkuro · 2018-02-27T18:02:24Z

@eundoosong yes, please add it. Only I would add it after that if statement, i.e. as the last statement of the while loop (or alternatively the first statement).

Re jaeger:reporter_spans{'result': 'socket_error} counter, I am not sure it's worth having it separate from the err counter, as it makes aggregations less obvious.

replace socker_error by reporter failure counter. Signed-off-by: Eundoo Song <eundoo.song@gmail.com>

eundoosong · 2018-02-28T15:59:40Z

@yurishkuro @jpkrohling reporter_socket(removed) is replaced by reporter_failure and reporter_queue_length is moved.
Is this ready to be merged, or is there anything else missing?

jpkrohling

LGTM, but I can't judge the Python part :)

yurishkuro

one small issue with update sampler metric

yurishkuro · 2018-03-06T17:17:11Z

jaeger_client/sampler.py

                self.error_reporter.error(
                    'Fail to update sampler'
                    'from jaeger-agent: %s [%s]', e, response)

    def _update_adaptive_sampler(self, per_operation_strategies):
        if isinstance(self.sampler, AdaptiveSampler):
            self.sampler.update(per_operation_strategies)
+            self.metrics.sampler_updated(1)


this should always run, for else: part as well

…ondition Signed-off-by: Eundoo Song <eundoo.song@gmail.com>

eundoosong · 2018-03-06T18:00:08Z

@yurishkuro moved sampler_updated metric out of the condition

yurishkuro · 2018-03-06T18:32:26Z

@eundoosong have you tried this in a real service with a real metrics library?

eundoosong · 2018-03-06T18:48:26Z

@yurishkuro I tried this using my prometheus factory as you see my first comment above
code: eundoosong@78aa262#diff-08bb54ec260941e2becb5054785b7fa2
It was for test, but I'm planning to use this in a real service with Prometheus and Grafana.

yurishkuro · 2018-03-06T23:54:24Z

Thanks, @eundoosong!

Do you think you can add a PR for PrometheusMetricsFactory? It think it would be great to have it in Jaeger client. The only gotcha is that we don't want to have a runtime dependency on prometheus client, so I would design the PrometheusMetricsFactory to import prom client conditionally (blow up if not found).

eundoosong · 2018-03-07T04:06:21Z

@yurishkuro Sure, I can add PrometheusMetricsFactory. PR will be created after tidying code and a try to remove a runtime dependency on prometheus client.
And I'd love to contribute the same in nodejs since our another main language for tracing client is nodejs.
related issue: jaegertracing/jaeger-client-node#178

eundoo-song and others added 2 commits February 13, 2018 19:19

Make tracer metrics consistent with Go client

760f560

Signed-off-by: eundoo-song <eundoo.song@samsung.com>

Fix lint error, add SamplerMetrics for the consistency

11308c3

Signed-off-by: Eundoo Song <eundoo.song@gmail.com>

add comment in reporter, sampler metrics class

9931342

Signed-off-by: Eundoo Song <eundoo.song@gmail.com>

add reporter_queue_length gauge

4880edf

Signed-off-by: Eundoo Song <eundoo.song@gmail.com>

move reporter_queue_length gauge as the last statement of while loop.

3002240

replace socker_error by reporter failure counter. Signed-off-by: Eundoo Song <eundoo.song@gmail.com>

jpkrohling approved these changes Feb 28, 2018

View reviewed changes

Merge branch 'master' into consistent_metrics

828843c

yurishkuro approved these changes Mar 6, 2018

View reviewed changes

in _update_adaptive_sampler, move sampler_updated metric out of the c…

421795e

…ondition Signed-off-by: Eundoo Song <eundoo.song@gmail.com>

Merge branch 'master' into consistent_metrics

a2a4b17

yurishkuro merged commit 8ddfcc1 into jaegertracing:master Mar 6, 2018

eundoosong mentioned this pull request Mar 7, 2018

Make tracer metrics consistent with Go client jaegertracing/jaeger-client-node#178

Closed

This was referenced Mar 16, 2018

Add PrometheusMetricsFactory #140

Closed

Add PrometheusMetricsFactory #142

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Metrics consistent with Go client. #129

Make Metrics consistent with Go client. #129

eundoosong commented Feb 24, 2018 •

edited by yurishkuro

Loading

coveralls commented Feb 24, 2018 •

edited

Loading

eundoosong commented Feb 24, 2018 •

edited

Loading

yurishkuro commented Feb 25, 2018

jpkrohling commented Feb 26, 2018

eundoosong commented Feb 27, 2018

jpkrohling commented Feb 27, 2018

eundoosong commented Feb 27, 2018

yurishkuro commented Feb 27, 2018

eundoosong commented Feb 28, 2018

jpkrohling left a comment

yurishkuro left a comment

yurishkuro Mar 6, 2018

eundoosong commented Mar 6, 2018

yurishkuro commented Mar 6, 2018

eundoosong commented Mar 6, 2018 •

edited

Loading

yurishkuro commented Mar 6, 2018

eundoosong commented Mar 7, 2018

Make Metrics consistent with Go client. #129

Make Metrics consistent with Go client. #129

Conversation

eundoosong commented Feb 24, 2018 • edited by yurishkuro Loading

coveralls commented Feb 24, 2018 • edited Loading

eundoosong commented Feb 24, 2018 • edited Loading

yurishkuro commented Feb 25, 2018

jpkrohling commented Feb 26, 2018

eundoosong commented Feb 27, 2018

jpkrohling commented Feb 27, 2018

eundoosong commented Feb 27, 2018

yurishkuro commented Feb 27, 2018

eundoosong commented Feb 28, 2018

jpkrohling left a comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

yurishkuro Mar 6, 2018

Choose a reason for hiding this comment

eundoosong commented Mar 6, 2018

yurishkuro commented Mar 6, 2018

eundoosong commented Mar 6, 2018 • edited Loading

yurishkuro commented Mar 6, 2018

eundoosong commented Mar 7, 2018

eundoosong commented Feb 24, 2018 •

edited by yurishkuro

Loading

coveralls commented Feb 24, 2018 •

edited

Loading

eundoosong commented Feb 24, 2018 •

edited

Loading

eundoosong commented Mar 6, 2018 •

edited

Loading