Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

trace() fails to execute block when zipkin disabled or not initialized. #35

Open
ParensAllTheWayDown opened this issue Jul 1, 2022 · 5 comments

Comments

@ParensAllTheWayDown
Copy link

I'm trying to ensure that our fastapi application continues to execute instrumented block if zipkin is/isnot configured. In the 'isnot' scenario, zipkin trace() scope function fails to execute the scope because get_tracer() returns a None and subsequent call to tracer.new_child fails.

    def __enter__(self) -> "trace":
        tracer = get_tracer()
        parent = _cur_span_ctx_var.get()
        if parent is None:
            parent = get_root_span()
        self._span = tracer.new_child(parent.context)
        self._tok = _cur_span_ctx_var.set(self._span)
        self._span.__enter__()
        self._span.name(self._name)
        self._span.kind(self._kind)
        return self

Would it be reasonable to fall back to an empty __enter__ & __exit__ if there is no tracer available? That would allow the instrumented code to run. Perhaps a warning message that indicates that things are not proceeding as expected.

I can work around this, but I have to duplicate the instrumented scope in a try-except in order to ensure that the instrumented code is executed in the case that zipkin configuration has been intentionally skipped. For example:

    @property
    def userid(self):
        try:
            with trace('ad_provider', kind='SERVER'):
                return self.get_user_from_validation_info()
        except AttributeError as ae:
            return self.get_user_from_validation_info()
@mchlvl
Copy link
Owner

mchlvl commented Jul 2, 2022

Hey @ParensAllTheWayDown ! Thanks for the suggestion! It sounds reasonable to me, so I prepared #36 that might help with this.

Just to make sure about the use cases - am I correctly assuming this happens when you do not add the middleware (that is skip app.add_middleware(ZipkinMiddleware)), or is there another situation when this happens?

@ParensAllTheWayDown
Copy link
Author

Short answer 'Yes'.

Longer answer follows: We are using 'Settings' subclass to define all settings for the application. If the zipkin subsection is not included, then ZipkinMiddleware is not included in our middleware stack.

class ZipkinSettings(BaseModel):
    """
    Configure Zipkin middleware layer
    """
    host: str = Field('127.0.0.1', description="The zipkin database that will collect call metrics.")
    port: int = Field(9411, description='The port to send performance metrics.')
    service_name: str = Field('gdmlapi', description='the name that identifies this gdmlapi server.')
    sample_rate: float = Field(1.0)
    inject_response_headers: bool = Field(True)
    force_new_trace: bool = Field(False)
    json_encoder: Callable = Field(json.dumps)
    header_formatter = Field(B3Headers)

and Settings subclass:

class Settings(BaseSettings):
    ...
    zipkin: Optional[ZipkinSettings]
    ...
    class Config:
        env_nested_delimiter = '__'

Our middleware include/exclude logic includes this:

        if settings.zipkin:
            config = ZipkinConfig(
                host=settings.zipkin.host,
                port=settings.zipkin.port,
                service_name=settings.zipkin.service_name,
                sample_rate=settings.zipkin.sample_rate,
                inject_response_headers=settings.zipkin.inject_response_headers,
                force_new_trace=settings.zipkin.force_new_trace,
                json_encoder=settings.zipkin.json_encoder,
                header_formatter=settings.zipkin.header_formatter
            )
            logger.debug(f"Adding zipkin: {config}")
            app.add_middleware(ZipkinMiddleware, config=config)

@mchlvl
Copy link
Owner

mchlvl commented Jul 5, 2022

Alright, thanks for clarifying, will make a new version with this later today 🙌

@mchlvl
Copy link
Owner

mchlvl commented Jul 5, 2022

@ParensAllTheWayDown Just merged and released 0.2.4. Let me now if this helps your usecase!

@ParensAllTheWayDown
Copy link
Author

Thank you for taking this into consideration. I'll give it a try tomorrow.

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

No branches or pull requests

2 participants