search

mchlvl / starlette-zipkin

Zipkin client for asgi. Compatible with Starlette Framework and Jaeger tracing server
10 stars 5 forks source link

FIX support external tracer (for tests) as well as an internally manged one #27

Closed mchlvl closed 2 years ago

mchlvl commented 2 years ago

As described in https://github.com/mchlvl/starlette-zipkin/pull/25 by @luminoso, currently we keep creating new az.Tracer on every request, while never closing any of them. This was not originally intended, as we planned to only create the az.Tracer once and reuse it for every request (while granting easy access via exposed context variable).

That would be similar to creating the az.Tracer with az.create(...) in the example setup: https://github.com/aio-libs/aiozipkin/blob/master/examples/aiohttp_example.py#L57

This PR solves the leak by allowing for externally provided tracer (for example used in tests), or internally managed one (first request creates the az.Tracer and ZipkinMiddleware instance holds its state.

codecov-commenter commented 2 years ago

Codecov Report

Merging #27 (375bf01) into master (7b92ffd) will decrease coverage by 0.50%. The diff coverage is 55.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #27      +/-   ##
==========================================
- Coverage   86.21%   85.71%   -0.51%     
==========================================
  Files           8        8              
  Lines         283      287       +4     
==========================================
+ Hits          244      246       +2     
- Misses         39       41       +2
Impacted Files Coverage Δ
starlette_zipkin/trace.py 89.74% <ø> (+1.13%) :arrow_up:
starlette_zipkin/middleware.py 82.07% <55.55%> (-2.09%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7b92ffd...375bf01. Read the comment docs.

mardiros commented 2 years ago

@mchlvl

From what I understand of the current issue, the proper fix is https://github.com/mchlvl/starlette-zipkin/pull/28

I DO NOT INVESTIGATE ENOUGH (functionally try to reproduce the issue and tests), I will do that tomorrow, and get back to you.

mchlvl commented 2 years ago

closed in favor of https://github.com/mchlvl/starlette-zipkin/pull/28