Inconsistent uuid_length parameter between CorrelationIdFilter and CeleryTracingIdsFilter

[dapryor]

Issue

There is a very similar interface between CorrelationIdFilter and CeleryTracingIdsFilter that caused confusion and also limits feature parity between the modules log filters.

class CorrelationIdFilter(Filter):
    """Logging filter to attached correlation IDs to log records"""

    def __init__(self, name: str = '', uuid_length: Optional[int] = None):
        super().__init__(name=name)
        self.uuid_length = uuid_length

 def filter(self, record: 'LogRecord') -> bool:
        """
        Attach a correlation ID to the log record.

        Since the correlation ID is defined in the middleware layer, any
        log generated from a request after this point can easily be searched
        for, if the correlation ID is added to the message, or included as
        metadata.
        """
        cid = correlation_id.get()
        if self.uuid_length is not None and cid:
            record.correlation_id = cid[: self.uuid_length]
        else:
            record.correlation_id = cid
        return True

class CeleryTracingIdsFilter(Filter):
    def __init__(self, name: str = '', uuid_length: int = 32):
        super().__init__(name=name)
        self.uuid_length = uuid_length

    def filter(self, record: 'LogRecord') -> bool:
        """
        Append a parent- and current ID to the log record.

        The celery current ID is a unique ID generated for each new worker process.
        The celery parent ID is the current ID of the worker process that spawned
        the current process. If the worker process was spawned by a beat process
        or from an endpoint, the parent ID will be None.
        """
        pid = celery_parent_id.get()
        record.celery_parent_id = pid[: self.uuid_length] if pid else pid
        cid = celery_current_id.get()
        record.celery_current_id = cid[: self.uuid_length] if cid else cid
        return True

as you can see above, the first filter takes an Optional[int] and the second filter takes an int.

There were two main problems we had with this

1. This caused some confusion due to the assumption that both filters followed the same interface.
2. CeleryTracingIdsFilter only supports trimming of the IDs. This is unlike CorrelationIdFilter which allows None to be passed to the uuid_length and it passes the untrimmed ID to the log.

This also relates to #50 since i am attempting to allow the actually celery task id to be used which is in uuid str form. This is 36 characters long due to the - characters, which leads to CeleryTracingIdsFilter truncating thiis uuid by default

Solution

The solution can be done in two parts.

1. change the uuid_length type on CeleryTracingIdsFilter to Optional[int] and do not trim the string when `None is passed
2. make the signature of CeleryTracingIdsFiltermatch CorrelationIdFilter by defaulting to None

The reason this is suggested to be a two part fix is because the second point would break existing code when users did not explicitly define uuid_length

[sondrelg]

Yeah looks like we changed the signature for the middleware log filter here https://github.com/snok/asgi-correlation-id/pull/14/files, and forgot to alter the celery one :woozy_face:

I agree they should match, but let's dig into exactly how thing might break existing apps if we do it in one go. I'm at the end of a long-ish day, so I might be missing something obvious here, but I can't really see how it would:

• The default length of a uuid.hex is 32 (which is what we're generating in the celery code, right?)
• Setting None (default) would mean we don't trim, which means the IDs would remain 32 chars
• If someone has specified a value, let's say 8, then that would still trim the IDs to 8 chars

Could you specify exactly why it would be a breaking change?

[sondrelg]

If you agree, I vote we just change the signature and call it a bugfix

[dapryor]

I'll go with whatever you say haha.

I think you are right in that it wont immediately break, but it could cause confusion in scenarios where someone was under the assumption that leaving uuid_length blank defaulted to not trimming.

ill put up a separate PR for fixing the signature unless someone else gets to it first.