snok / asgi-correlation-id

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

Supporting multiple header fields #81

Closed alfechner closed 6 months ago

alfechner commented 6 months ago

We use both, X-Request-ID as well as X-Correlation-ID in our services for legacy reasons like this:

So X-Correlation-ID has precedence over X-Request-ID.

Do you see your library supporting this kind of logic?

Thanks for your work!

sondrelg commented 6 months ago

I suppose it wouldn't be very hard to extend the type of the header_name argument to take a str or list[str]. I guess the first specified header key taking precedence would also be fairly intuitive.

So we would allow both today's setup:

app.add_middleware(
    CorrelationIdMiddleware,
    header_name='X-Request-ID',
    update_request_header=True,
    generator=lambda: uuid4().hex,
    validator=is_valid_uuid4,
    transformer=lambda a: a,
)

and also extend the package to allow:

app.add_middleware(
    CorrelationIdMiddleware,
    header_name=['X-Correlation-ID', 'X-Request-ID'], 
    update_request_header=True,
    generator=lambda: uuid4().hex,
    validator=is_valid_uuid4,
    transformer=lambda a: a,
)

Where keys are checked by the order of the list.

Can you think of any pitfalls or other considerations worth making? Is this how you imagined it?

alfechner commented 6 months ago

100% what I was thinking of. Thanks for the quick answer!

sondrelg commented 6 months ago

Would you be interested in creating a PR?

alfechner commented 6 months ago

Sure, I might need some days, guess I'll find some time between the years.

alfechner commented 6 months ago

I'm wondering how we go about updating the headers of the request and the response.

Our use case is that some legacy projects sent X-Request-ID and it's therefore considered. We'd like to update X-Correlation-ID and leave X-Request-ID alone but that could be different for other use cases.

So there are 4 things to define:

  1. What is the source of the correlation_id?
  2. Update the request headers?
  3. What request headers to update?
  4. What response headers to update?
class CorrelationIdMiddleware:
    app: 'ASGIApp'
    header_name: str | list[str] = 'X-Request-ID'
    # update_request_header: bool = True
    update_request_header_name: Optional[str | list[str]] = 'X-Request-ID' # None = don't update
    update_response_header_name: str | list[str] = 'X-Request-ID'

Empty lists could also indicate don't update and would technically have the same effect. In this case None could refer to "use everything you've got in header_name".

Wdyt?

sondrelg commented 6 months ago

Thinking about this again, could your use-case be solved by adding stacking two instances of the existing middleware?

# Run X-Request-ID first, since it will be overwritten by X-Correlation-ID if it exists
app.add_middleware(
    CorrelationIdMiddleware,
    header_name='X-Request-ID',
)
app.add_middleware(
    CorrelationIdMiddleware,
    header_name='X-Correlation-ID',
)
alfechner commented 6 months ago

I guess it would work except if X-Request-ID and X-Correlation-ID are both set because then X-Request-ID would take precedence over X-Correlation-ID. That's because the second MW doesn't respect the fact, that the first MW was able to resolve the correlation id already.

Anyway, that's probably an edge case. I need to discuss with my team in January. But I believe this should be fine for us.

I'll close this issue and come back to it if your proposal doesn't work for some reason.

Thanks a 1000 times for your work and your advice :)