Why would we need this? Can you give some examples or actual useful links that you would register here? I feel like that there are already too many buttons in the dialog.

Codecov Report

Merging #5094 into master will increase coverage by <.01%.
The diff coverage is 95.23%.

@@            Coverage Diff             @@
##           master    #5094      +/-   ##
==========================================
+ Coverage   78.46%   78.47%   +<.01%     
==========================================
  Files         467      467              
  Lines       29766    29782      +16     
==========================================
+ Hits        23357    23372      +15     
- Misses       6409     6410       +1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 603d02a...72793a9. Read the comment docs.

@Fokko I think it is up to the users to decide whether they need to add these extra links. One use case at Lyft is to add extra links to task logs in Kibana/Elastic search and we probably will add links to task logs that are archived in S3. Therefore we need a way other than #5059 to register operator extra links at global level.

PTAL @feng-tao @ashb

PTAL @XD-DENG @Fokko

I think it's a nice idea and can be kind of useful in some cases but I think I agree more with @Fokko.

@Fokko @feluelle
I am not too concerned that there will be too many buttons in the dialog UI. It is up to the user to decide whether they want to add more buttons. If the user thinks that they don't want those buttons, they simply don't register any. I would rather provide the more flexibility to the users than limit it.

Would like to hear more about the reason behind that against such flexibility.

@Fokko @feluelle , I agree with @milton0825 that it would be good to provide the flexibility instead limiting that. In fact we have been running this feature at Lyft in production for long time(https://eng.lyft.com/running-apache-airflow-at-lyft-6e53bb8fccff). E.g one use case could be make the kibana log url for all the operators as kibana log searching is much easier to look at the Airflow log.

tests passed on: https://travis-ci.org/milton0825/airflow/builds/522373084

@Fokko , @feluelle happy to address any other concerns you guys have. Let us know.

@Fokko @feluelle let us know if you have any concerns~

@milton0825 @feng-tao actually I didn't know that we already had #5059 in.

I am not too concerned that there will be too many buttons in the dialog UI. It is up to the user to decide whether they want to add more buttons. If the user thinks that they don't want those buttons, they simply don't register any. I would rather provide the more flexibility to the users than limit it.

Thanks Chao-Han. I absolutly agree. I also have thought about that feature once again and you convinced me. Please count my vote as +1 now.

The only issue I still have is the right location for those buttons. I think there could be a better dialog window for those buttons. Maybe we add one dialog window for operator options where we could then place those like we have for task instances.

thanks @feluelle . @Fokko , let us know if you still have concern, @milton0825 , I will take a closer look at the actual implementation tonight.

Quick question about this implementation. Could you help provide an example usage for let's say Kibana logs that require arguments to be passed to it. For example, if I want to add a global Kibana link for all tasks, I need to be able to specify something like a log ID to the URL to redirect to the right place. Does this implementation allow that?

@kunalyogenshah so currently the operator instance and the execution_time are exposed to you like in: https://github.com/apache/airflow/pull/5094/files#diff-2fdb519812514e29589938d977d68ea0R94 and you should have the information (dag_id, task_id,...) to formulate the URL with granularity to each task_instance. If you need information such as log id that is out of the scope of airflow, you can do whatever you need in the get_link to get that information.

@milton0825 Got it. Constructing it this way is certainly doable. But somehow it does seem like introducing the Kibana log URLs in a more central way like the Download log links / View log links could be more semantically appropriate than adding them through global extra links? Specially if you consider adding try number to the mix, in which case a Kibana global should return multi-links instead of one link. Just a thought. + @KevinYang21

Nice idea to make it pluggable. We do also have the need to put a Kibana link on the dialog but as @kunalyogenshah mentioned, for task logs I think it makes more sense to provide different links for different try_number. Possible that we can make it so flexible that we can have plugin section just like the download log section?

@KevinYang21 @kunalyogenshah I think it totally make sense to include the try-number into the scope.

I am thinking about addressing that in a separate PR to limit the scope of changes in this PR and here is the jira to track that: https://issues.apache.org/jira/browse/AIRFLOW-4398

@milton0825 , is there a reason we don't make all the fields available(https://github.com/apache/airflow/blob/master/airflow/models/taskinstance.py#L126-L144) or at least most of the fields? I would imagine that adding pool / priority_weight to the task will be useful as well.

@feng-tao we exposed the operator so all the following fields should be available to user:
https://github.com/apache/airflow/blob/8be65327e8cd1f4cf7dedcb90b8eacc4fa5f635d/airflow/models/baseoperator.py#L241-L274

@feng-tao Took a quick look at the code and I think that's because we start to construct the links from operator instead of from task instance. But since we already have the execution_date we can always construct the TI just like other endpoints and use whatever info is inside.

While getting the TI fields is easy, my thinking is still more on the form of the link. The fact that we can just construct a link button right now is probly not going to look very nice when we need some other format, like when we want the download log look for our kibana log links.

EDIT: (hit comment button too soon) I'm not trying to scope out this PR, I think there might be use cases that a link button is enough. If it is not trivial to create such a framework for more complex pluggable components in the dialog it makes sense to start another PR around that.

yeah, FORM/Design/UI is always a tough question....

BTW @KevinYang21 , is [email protected] your email ?

@feng-tao yes it is.

@KevinYang21 , not sure how to DM you, but you may want to check your email :)

@feng-tao this is ready for another look
tests passed on: https://travis-ci.org/milton0825/airflow/builds/523800135

@Fokko let us know if you have any other concerns.