[Webserver] Use operator_name instead of task_type in UI

[utkarsharma2]

we should use operator_name when displaying operator name in UI.

Related: #28466 (not closing yet, see discussion below)

[utkarsharma2]

Found one more instance of task_type usage in UI, working on changes accordingly.

[utkarsharma2]

@uranusjr While going through the code I found there are two other tooltips where we show the operator name

airflow/airflow/www/static/js/task_instances.js

Line 111 in 29d2a31

tt += `Operator: ${escapeHtml(ti.operator)}<br>`;

and

airflow/airflow/www/static/js/task_instances.js

Line 170 in 29d2a31

tt += `Operator: ${escapeHtml(ti.operator)}<br>`;

Ideally, both of them should show values derived from custom_operator_name but only the base operator has the operator_name property and not the task instance object. In both of the instance as mentioned above, we are displaying values of ti.operator which are populated from task_type of operator.

airflow/airflow/models/taskinstance.py

Line 888 in 29d2a31

self.operator = task.task_type

since self.operator is getting used at multiple places in code, I think we should create another property in the task instance operator similar to operator_name in the base operator and use that while displaying in UI.

However, this change can be a separate PR. WDYT?

[uranusjr]

Hmm, yeah adding a new field is probably the simplest approach. Can you find other uses of TaskInstance.operator so maybe we can analyse if this field is simply for UI (if so, we can just change it to use operator_name), or do other logic depend on it? Adding a field is relatively expensive on TaskInstance so we should avoid that if possible.

Either way, I think we should hold this PR as-is and continue looking into TaskInstance.operator in a different PR.

[uranusjr]

Do we still need this? If the only use case if to check SubDagOperator, we can use operator_name as well.

[utkarsharma2]

Yes, I think we still need this since the below code compares the operator class name and it's best to keep this since the operato_name can change.

airflow/airflow/www/static/js/graph.js

Line 211 in 8891c69

isSubDag: task.task_type === "SubDagOperator",

and based on that displaying the subdag button -

airflow/airflow/www/static/js/callModal.js

Lines 185 to 190 in 8891c69

	if (isSubDag) {
	$("#div_btn_subdag").show();
	subDagId = `${dagId}.${taskId}`;
	} else {
	$("#div_btn_subdag").hide();
	}

[bbovenzi]

Wouldn't it be better to pass is_subdag from the webserver instead? That's what we do in the REST API

[utkarsharma2]

@bbovenzi Would it make sense to raise a separate PR for this change? because I think it's a separate task.

[jedcunningham]

Yeah, let's do a separate PR.

[utkarsharma2]

Hmm, yeah adding a new field is probably the simplest approach. Can you find other uses of TaskInstance.operator so maybe we can analyze if this field is simply for UI (if so, we can just change it to use operator_name), or do other logic depends on it? Adding a field is relatively expensive on TaskInstance so we should avoid that if possible.

Either way, I think we should hold this PR as-is and continue looking into the TaskInstance.operator in a different PR.

@uranusjr I checked for the usage of ti.operator and found out there are a lot of places where it's been used, below are a few examples

airflow/airflow/models/dagrun.py

Line 1126 in 8891c69

created_counts[ti.operator] += 1

airflow/airflow/models/dag.py

Line 1749 in 8891c69

for ti in query.filter(TI.operator == ExternalTaskMarker.__name__):

airflow/airflow/jobs/scheduler_job_runner.py

Line 744 in 8891c69

ti.operator,

TaskInstance.operator certainly has more usage than just displaying in UI.

I think adding another field to TaskInstance will be a quick solution. WDYT?

[uranusjr]

Yes that’s probably best. I think some of the use cases can technically switch to operator_name (e.g. logging), but at least the second one you mentioned should use the raw class name.

[utkarsharma2]

@uranusjr Added a new property custom_operator_name in TaskInstance class.

Also, would be nice if you can cross verify all the places that should be populating the value for custom_operator_name are covered.

[utkarsharma2]

@uranusjr since there is a new field added to TaskInstance class which requires a new column to be created in TaskInstance table. I think this would be a breaking change, right? Do we have something that does database migration similar to Django migrations? if so, we can add a default value so that the database migrations can be smooth for existing TaskInstance.

[potiuk]

@uranusjr since there is a new field added to TaskInstance class which requires a new column to be created in TaskInstance table. I think this would be a breaking change, right? Do we have something that does database migration similar to Django migrations? if so, we can add a default value so that the database migrations can be smooth for existing TaskInstance.

We have alembic. You can see the docs about addin new migrations (look for alembic) and you can also take a look at past prs that added migrations to airflow\migrations. Also worth looking at the airflow db command and all their subcommands and https://airflow.apache.org/docs/apache-airflow/stable/migrations-ref.html

[jedcunningham]

Yeah, let's do a separate PR.