Remove TaskContextLogger #43183
Remove TaskContextLogger #43183
Conversation
It was a fine idea, but we ultimately realized that it's better to just add Log records.
What are the "Log records" you are mentioning, could you expand a little |
The Also knows as |
dstandish
added
the
airflow3.0:breaking
| @@ -238,10 +237,6 @@ def set_context(self, ti: TaskInstance, *, identifier: str | None = None) -> Non | |||
| self.handler.setLevel(self.level) | |||
| return SetContextPropagate.MAINTAIN_PROPAGATE if self.maintain_propagate else None | |||
|
|
|||
| @cached_property | |||
| def supports_task_context_logging(self) -> bool: | |||
| return "identifier" in inspect.signature(self.set_context).parameters | |||
There was a problem hiding this comment.
Do we intend to keep accepting the identifier param in the set_context method above that we had introduced in https:/apache/airflow/pull/32646/files. The docstring for that param in the method is currently tightly related to the task context logger.
This param was also extended to be used in the set_context method of the provider log handlers with the below PRs
- AWS S3: Extend task context logging support for remote logging using AWS S3 #32950
- GCP GCS: Extend task context logging support for remote logging using GCS #32970
- Azure WASB: Extend task context logging support for remote logging using WASB (Azure Blob Storage) #32972
- ElasticSearch: Extend task context logging support for remote logging using Elasticsearch #32977
It seems the conditionals around AF versions in those provider PRs were removed & cleaned up when those providers started having Airflow min version as 2.8 -- in PR #42764
If we decide to drop the param, perhaps would also be a good idea to add a TODO in those providers saying that this param would no longer be needed/used when min AF version is 3.0 & can be cleaned up.
There was a problem hiding this comment.
It's a good point. I thought about it and figured that it's ok to leave it even if it is not currently used. Unless there's some backcompat issue with leaving it but i don't think there is.
Do you see any other backcompat concerns with this PR otherwise?
There was a problem hiding this comment.
Yeah. I think we should drop it - and actually - if we are going to add documentation for Airlfow 3, I think we should promote and look more closely into open-telemetry trace integration, which is essentially doing exactly what have been done here (and more).
Open Telemetry span is equivalent to the identifier, but there is already tooling (all tools that support open-telemetry spans) that understand and are able to visualise spans across distributed systems. So I'd say we should make it clearer and explain to people on how they can connect those tools to see relations between things like errors raised in other parts of the system that are related to particular tasks.
This was a very important point raised in the debugging survey - that it is difficult to debug issues in a distributed environment, and IMHO open-telemetry and better integration with it is the answer to that need and maybe we shoudl somehow think about making it more obvious (and learn ourselves how to do it).
There was a problem hiding this comment.
Do you see any other backcompat concerns with this PR otherwise?
I can't think of any backcompat issues otherwise :), just felt that we would have there an unused param and thought to highlight it once.
It was a fine idea, but we ultimately realized that it's better to just add records to the
Logtable, which can be viewed easily in the UI