Keep the tracer reference in the middleware itself

[mardiros]

So I checkout tag 0.1.2 and then tag 0.2 and your branch.

In 0.1.2, there where a ZipkinMiddleware.tracer attribute initialize on the init_tracer function

    async def init_tracer(self) -> None:
        if self.tracer is None:
            endpoint = az.create_endpoint(self.config.service_name)
            tracer = await az.create(
                f"http://{self.config.host}:{self.config.port}/api/v2/spans",
                endpoint,
                sample_rate=self.config.sample_rate,
            )
            self.tracer = tracer
            _tracer_ctx_var.set(tracer)
        else:
            _tracer_ctx_var.set(self.tracer)

and it was initialized on the first dispatch

    async def dispatch(
        self, request: Request, call_next: RequestResponseEndpoint
    ) -> Response:
        await self.init_tracer()
        tracer = get_tracer()

       ...
        with function(**kw) as span:
            try:
                ...
                return response
            finally:
                ...
        await tracer.close()

(I've also note that the tracer.close was an unreachable statement (not in finally) and thankfully because the tracer was not supposed to be closed.)

And, for SOLID principle, Ive move all the ContextVar manipulation in the trace.py module.

        tracer = get_tracer()
        if tracer is None:
            tracer = await init_tracer(self.config)

and the self.tracer was never initialized (and I should remove it completly.

From what I understand, event after and init_tracer(self.config) the get_tracer() return None and we reinit it ?

If it is the case, I think we can keep the reference in the middleware instance like in this merge request.

I have to do more investigation to confirm that.

[codecov-commenter]

Codecov Report

Merging #28 (69acb22) into master (39e485e) will increase coverage by 1.93%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #28      +/-   ##
==========================================
+ Coverage   86.21%   88.15%   +1.93%     
==========================================
  Files           8        8              
  Lines         283      287       +4     
==========================================
+ Hits          244      253       +9     
+ Misses         39       34       -5     

Impacted Files	Coverage Δ
starlette_zipkin/middleware.py	85.98% <100.00%> (+1.82%)	⬆️
starlette_zipkin/trace.py	93.50% <100.00%> (+4.89%)	⬆️

---

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 39e485e...69acb22. Read the comment docs.

[mchlvl]

Here the _tracer_ctx_var is not persisted between requests, so we still have to somehow to set it for every subsequent requests

[mardiros]

oh, ok! Got it. thanks

[mardiros]

@mchlvl
Now I really understand how it is supposed to works.

We keep the reference of the tracer in the middleware, and push it in the context on every requests.
And it is supposed to be works exactly like the root span.

As a better solution, one context var could be used for both variable because they are used together.

69acb22

[mchlvl]

It was definitely not correct in 0.1.2, no doubt about that :D Thanks for making this proposal, looks good! Other than that one comment, I think the tests will need adjustment to properly use the tracer fixture

[mardiros]

It was definitely not correct in 0.1.2, no doubt about that :D Thanks for making this proposal, looks good! Other than that one comment, I think the tests will need adjustment to properly use the tracer fixture

No offense, It was just for explaining the whole context of the changes.

I think it is ok now with this merge request.

The example works fine.

[luminoso]

Is this branch ready for merge? If so, I'll do some tests asap.

[mchlvl]

@mardiros neat! I noted the tests are soft complaning, but I think that should not block the functionality, so lets get this in 🎉

[luminoso]

Manually tested. Traces look ok! 👍