Skip to content

Conversation

@IAL32
Copy link
Contributor

@IAL32 IAL32 commented Jan 13, 2023

It is possible to view the current job run in the AWS Console.

I add a log pointing to the URL that shows the current job run for convenience.

I also slightly modify the test to include job use a job name with slash, which is a valid job name.

@boring-cyborg boring-cyborg bot added area:providers provider:amazon AWS/Amazon - related issues labels Jan 13, 2023
@Taragolis
Copy link
Contributor

@IAL32 I would suggest to create external link:
All current links in AWS Provider: https://github.com/apache/airflow/tree/main/airflow/providers/amazon/aws/links
Latest PR which add external links to operator: #28180

You could keep also link within the logs sometime it also useful, however I think external link much useful

@IAL32
Copy link
Contributor Author

IAL32 commented Jan 13, 2023

I was not aware of the functionality. Neat! I added it and the test for it, let me know what you think 😄

Comment on lines 22 to 26
class GlueJobLogsLink(BaseAwsLink):
"""Helper class for constructing AWS Glue Job Logs Link"""

name = "AWS Glue Job Logs"
key = "glue_job_logs"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as i understand you want to add link to AWS Glue Studio ❯ Monitoring ❯ Job run not link to Cloudwatch logs

So better named it appropriate like (it suggested renaming, final choice by you):
GlueJobLogsLink > GlueJobRunDetailsLink
"AWS Glue Job Logs" > "AWS Glue Job Run Details"
"glue_job_logs" > "glue_job_run_details"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might also make sense to add the CloudWatch logs link?
Maybe I will add them on a new PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, why not? We already add some similar for BatchOperator.

I think for Glue it might required create a bit different link, e.g. Batch use link to a single LogStream, but Glue create multiple different log stream during execution so potentially we need to provide link to filter LogStream by specific Job ID

Copy link
Contributor

@Taragolis Taragolis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

Comment on lines 152 to 156
glue_job_run_url = (
f"{BASE_AWS_CONSOLE_LINK}/gluestudio/home?"
+ f"region={glue_job.conn_region_name}#/job/{urllib.parse.quote(self.job_name, safe='')}/run/"
+ glue_job_run["JobRunId"]
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

Could you not reuse the format string from the Link class so as to not duplicate it here? The two could easily get out of sync. Something like (NOTE: untested):

Suggested change
glue_job_run_url = (
f"{BASE_AWS_CONSOLE_LINK}/gluestudio/home?"
+ f"region={glue_job.conn_region_name}#/job/{urllib.parse.quote(self.job_name, safe='')}/run/"
+ glue_job_run["JobRunId"]
)
glue_job_run_url = GlueJobRunDetailsLink.format_str.format(
region_name=glue_job.conn_region_name,
job_name=urllib.parse.quote(self.job_name, safe=''),
job_run_id=glue_job_run["JobRunId"])

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @o-nikolas for the suggestion! We should consider having a similar signature of .persist to also get the formatted link. My approach seems a bit ugly and forced.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be an interesting PR to work for 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not? When I initially create BaseAwsLink I've just grab an idea from BaseGoogleLink

@o-nikolas o-nikolas merged commit 395b731 into apache:main Jan 19, 2023
maggesssss pushed a commit to maggesssss/airflow that referenced this pull request Jan 21, 2023
* Log AWS Glue Job Console URL

* Added GlueJobLogsLink
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers provider:amazon AWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants