You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
There is a very similar interface between CorrelationIdFilter and CeleryTracingIdsFilter that caused confusion and also limits feature parity between the modules log filters.
classCorrelationIdFilter(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_lengthdeffilter(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()
ifself.uuid_lengthisnotNoneandcid:
record.correlation_id=cid[: self.uuid_length]
else:
record.correlation_id=cidreturnTrue
classCeleryTracingIdsFilter(Filter):
def__init__(self, name: str='', uuid_length: int=32):
super().__init__(name=name)
self.uuid_length=uuid_lengthdeffilter(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] ifpidelsepidcid=celery_current_id.get()
record.celery_current_id=cid[: self.uuid_length] ifcidelsecidreturnTrue
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
This caused some confusion due to the assumption that both filters followed the same interface.
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.
change the uuid_length type on CeleryTracingIdsFilter to Optional[int] and do not trim the string when `None is passed
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
The text was updated successfully, but these errors were encountered:
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?
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.
Issue
There is a very similar interface between
CorrelationIdFilter
andCeleryTracingIdsFilter
that caused confusion and also limits feature parity between the modules log filters.as you can see above, the first filter takes an
Optional[int]
and the second filter takes anint
.There were two main problems we had with this
CeleryTracingIdsFilter
only supports trimming of the IDs. This is unlikeCorrelationIdFilter
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 toCeleryTracingIdsFilter
truncating thiis uuid by defaultSolution
The solution can be done in two parts.
CeleryTracingIdsFilter
toOptional[int]
and do not trim the string when `None is passedCeleryTracingIdsFilter
matchCorrelationIdFilter
by defaulting toNone
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
The text was updated successfully, but these errors were encountered: