Skip to content
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

SEAB-5860: Convert "ecs stopped task" event to Slack message #160

Merged

Conversation

svonworl
Copy link
Contributor

@svonworl svonworl commented Jan 18, 2024

Description
This PR modifies the "cloudwatch-to-slack" lambda to produce a "ecs task stopped" Slack message when it processes an event that indicates such. The companion deploy PR, which modifies DockstoreStack to send the event to the associated SNS topic, is https://github.com/dockstore/dockstore-deploy/pull/746.

The Slack message looks like: https://ucsc-gi.slack.com/archives/C02DTE86E8J/p1705536998257129

Note that subsequently, I removed the startedBy field, it will rarely change and didn't seem worth the space. We can nuke any of the other information if we don't think it is useful.

Charles mentions that it'd be useful to link items in this and other Slack messages to the appropriate AWS page. So, I investigated, and it's certainly doable, but not as easy as pasting a resource ARN onto the end of an AWS url. https://github.com/link2aws/link2aws.github.io purports to convert an ARN to an AWS page, and at first glance, the source looks ok, but I haven't tried it out. I recommend we defer the addition of links until later.

A message will be produced for each task that is scaled out of existence. Eventually, as more people use Dockstore, the scaling-related messages will become too numerous, and we'll probably want to filter them out, and only display information about tasks that were stopped for other reasons (due to an error, by administrative action, etc).

This PR took much longer for me, someone who has touched the deploy in mostly tangential/superficial fashion, than it would have for the primary deploy authors. None of it was difficult, but piecing together the how everything worked, and then how to test it, and executing on that, in enough detail to confirm that I didn't break anything else in the process, was super-duper time-consuming. Imagine me spending 5 minutes each figuring out 100 things that someone that wrote the code would've already known, that was basically how it went...

Review Instructions
See https://github.com/dockstore/dockstore-deploy/pull/746

Issue
https://ucsc-cgl.atlassian.net/browse/SEAB-5860

Security
Is the information that's contained in the message suitable for display in the target Slack channel?

Please make sure that you've checked the following before submitting your pull request. Thanks!

  • Ensure that the PR targets the correct branch. Check the milestone or fix version of the ticket.

@svonworl svonworl changed the title SEAB-5860: Convert "Stopped task" event to Slack message SEAB-5860: Convert "ecs stopped task" event to Slack message Jan 18, 2024
@svonworl svonworl self-assigned this Jan 18, 2024
@kathy-t
Copy link
Contributor

kathy-t commented Jan 18, 2024

Note that subsequently, I removed the startedBy field, it will rarely change and didn't seem worth the space. We can nuke any of the other information if we don't think it is useful.

I can see it being useful to see how long the task lived. For example, it would tell use if tasks were started then killed immediately or if a long-running task was killed

EDIT: I read that too fast and read startedBy as startedAt, please ignore this comment 😁

Copy link
Contributor

@kathy-t kathy-t left a comment

Choose a reason for hiding this comment

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

This is great - excited to see it in action. I think a bonus is that we'll see when AWS re-deploys our ECS service with security updates

@denis-yuen
Copy link
Member

None of it was difficult, but piecing together the how everything worked, and then how to test it, and executing on that, in enough detail to confirm that I didn't break anything else in the process, was super-duper time-consuming. Imagine me spending 5 minutes each figuring out 100 things that someone that wrote the code would've already known, that was basically how it went...

FWIW, we definitely appreciate it!

@kathy-t
Copy link
Contributor

kathy-t commented Jan 18, 2024

Should this PR be directed at the release/0.3.0 branch?

Copy link
Contributor

@coverbeck coverbeck left a comment

Choose a reason for hiding this comment

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

Second Kathy's comment about target branch.

}

function ecsTaskStateChangeMessageText(message) {
const taskArn = message?.detail?.taskArn;
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, for some reason I thought optional chaining was a TypeScript only feature. Good to know it's not. Looks like the feature was added to most browsers in 2020, so when I first started on Dockstore, it actually was TS-only, and I never noticed it was added to JS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh, for some reason I thought optional chaining was a TypeScript only feature. Good to know it's not. Looks like the feature was added to most browsers in 2020, so when I first started on Dockstore, it actually was TS-only, and I never noticed it was added to JS.

Looks like it got added to Node in version 14:
https://nodejs.medium.com/node-js-version-14-available-now-8170d384567e

@svonworl svonworl changed the base branch from develop to release/0.3.0 January 18, 2024 17:34
let messageText = `Task ${taskArn} is now ${lastStatus}`;
if (message?.detail != undefined) {
["startedAt", "stoppedAt", "stoppedReason"].forEach((name) => {
const value = message?.detail[name];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I browsed regarding optional chaining, I noticed that there's a syntax a?.[expr] that "optional chains" the expression-based lookup. I am going to make a quick mod to eliminate the outer if

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@svonworl svonworl merged commit a675de7 into release/0.3.0 Jan 18, 2024
12 checks passed
@denis-yuen denis-yuen deleted the feature/seab-5860/send-ecs-stopped-tasks-to-slack branch January 19, 2024 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants