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%)	:arrow_up:
starlette_zipkin/trace.py	93.50% <100.00%> (+4.89%)	:arrow_up:

---

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]

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! :+1: