-
Notifications
You must be signed in to change notification settings - Fork 3k
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
feat(ingest): enable EnsureAspectSizeProcessor
for all sources
#12262
Conversation
if _DATAHUB_EMITTER_TRACE: | ||
logger.debug(f"Attempting to emit MCP batch of size {len(mcps)}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could remove such repeated conditionals, how about simply adding new function to the logging, i.e. in logging_manager.py
:
def trace(self: logging.Logger, message: str, *args: Any, **kwargs: Any):
env = f'TRACE_{self.name.replace(".", "__").upper()}'
if get_boolean_env_variable(env, False):
self._log(logging.DEBUG, message, args, **kwargs)
logging.Logger.trace = trace
Then here we would be only calling logger.trace(...)
. the variable to be exported in this case would be:
export TRACE_DATAHUB__EMITTER__REST_EMITTER=true
It would be quite universal piece of code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately mypy really doesn't like this type of patching
I agree with you that the code duplication isn't ideal, but it's better than having type: ignore
everywhere. If there's an alternative that doesn't require that, I'm all ears
I agree with the spirit of the change with regards to the workunit processor placement. I think we should avoid repeated conditionals when debug logging, proposed a solution in a comment. |
Checklist