search

snok / asgi-correlation-id

Request ID propagation for ASGI apps
MIT License
369 stars 29 forks source link

Add public interface for getting current correlation id #53

Closed Bobronium closed 1 year ago

Bobronium commented 1 year ago

Currently one can

from asgi_correlation_id.context import correlation_id

current_correlation_id = correlation_id.get()

But it's unclear whether it's safe to rely on asgi_correlation_id.context.correlation_id since it's not added to the package's __init__.

Ways to address this

  1. Importasgi_correlation_id.context.correlation_id in package __init__ and add it to __all__, explicitly making it a part of public interface
  2. Create a function get_current_correlation_id(default: Any = None) -> str | None that returns current correlation_id and make it public

The first approach allows full control over correlation_id, which may be not desirable. Second one only exposes getting part which is a lot safer.

sondrelg commented 1 year ago

Yeah that's fair. Since the ID is needed to propagate it to other services, it should be a part of the public API. In django-guid the second approach was used, but after using it for a while I'm not a huge fan of it tbh. I personally think approach 1 is simpler and has no real downsides.

Any thoughts on this @JonasKs?

JonasKs commented 1 year ago

The only reason to add a public API/helper functions on top of the contextvars are for newcomers I think. See threads on SO like this one, where the question is literally how to change the contextvar value.

I think it would be better to document how it’s implemented, and link to the docs for contextvars, than to wrap it. I know many people don’t look at package implementation, and contexvars can feel a bit magical.

I 100% vote for option 1. :blush:

.. sneaky edits.

sondrelg commented 1 year ago

Want to create a PR for this @Bobronium? 🙂