[WX-1792] Helper to Actor #7544

THWiseman · 2024-09-16T14:02:51Z

Description

This PR refactors & renames the CostCatalogHelper into the PollResultMonitorActor. Doing this allows the helper to asynchronously communicate with the CostCatalogService, which it needs to do in order to calculate a VM Cost Per Hour.

Release Notes Confirmation

`CHANGELOG.md`

I updated CHANGELOG.md in this PR
I assert that this change shouldn't be included in CHANGELOG.md because it doesn't impact community users

Terra Release Notes

I added a suggested release notes entry in this Jira ticket
I assert that this change doesn't need Jira release notes because it doesn't impact Terra users

jgainerdewar · 2024-09-16T17:42:29Z

backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala

-  )
+  def onTaskComplete(runStatus: StandardAsyncRunState, handle: StandardAsyncPendingExecutionHandle): Unit =
+    pollingResultMonitorActor.foreach(helper =>
+      helper.tell(AsyncJobHasFinished(runStatus.getClass.getSimpleName), self)


Can we pass runStatus here rather than stringifying it?

jgainerdewar · 2024-09-16T17:50:13Z

...mon/src/main/scala/cromwell/backend/google/pipelines/common/PapiPollResultMonitorActor.scala

+}
+
+class PapiPollResultMonitorActor(tellMetadataFn: Map[String, Any] => Unit,
+                                 tellBardFn: (String, OffsetDateTime, Option[OffsetDateTime], OffsetDateTime) => Unit


Might be worth bringing back the StartAndEndTimes class, a series of OffsetDateTime can be easy to mess up.

jgainerdewar · 2024-09-16T17:55:39Z

...mon/src/main/scala/cromwell/backend/google/pipelines/common/PapiPollResultMonitorActor.scala

+      case event if event.name == CallMetadataKeys.VmEndTime => event.offsetDateTime
+    }
+
+  override def tellMetadata(metadata: Map[String, Any]): Unit = tellMetadataFn(metadata)


Can we give this actor a handle to the metadata service registry rather than having all the backends pass in (I assume identical?) implementations?

Similar question for Bard - can we put the actual message-passing to the Bard service in the parent PollResultMonitorActor and figure out the minimum logic that needs to be per-backend?

That is certainly possible, but I'm not sure if that's a big improvement since we would also need to pass in a lot of information about the particular task (runtime attributes, job descriptor, etc...) in order to have all the information necessary to send the message in the right way. All that other stuff doesn't seem relevant to tracking start and end times, yet we still want to send messages that include more context about their task. The passing of the function object is more about capturing necessary context from the parent class than it is about sharing implementation.

Yeah, I see what you mean, for Bard. Maybe we could have a method that takes the current inputs and creates and returns a TaskSummaryEvent, which the PollMonitorActor knows what to do with?

Updated branch to do something along these lines. We no longer pass in callbacks from individual backends, but instead pass in some extra data so that the poll monitors can call their own implementations of tellMetadata and tellBard instead. This allowed us to remove tellBard from elsewhere in the codebase.

THWiseman · 2024-09-17T17:41:19Z

backend/src/main/scala/cromwell/backend/standard/pollmonitoring/PollResultMonitorActor.scala

+  }
+
+  // Function that reports metrics to bard, called when a specific call attempt terminates.
+  def tellBard(terminalStateName: String,


Moved (almost) verbatim from StandardAsyncExecutionActor

jgainerdewar · 2024-09-17T18:36:40Z

...Backends/tes/src/main/scala/cromwell/backend/impl/tes/TesAsyncBackendJobExecutionActor.scala

@@ -322,7 +320,6 @@ object TesAsyncBackendJobExecutionActor {
                     handle: StandardAsyncPendingExecutionHandle,
                     getTaskLogsFn: StandardAsyncPendingExecutionHandle => Future[Option[TaskLog]],
                     tellMetadataFn: Map[String, Any] => Unit,
-                     tellBardFn: StandardAsyncRunState => Unit,


I'm going to create a ticket in this epic to track the fact that we've disabled Bard reporting here.

jgainerdewar

Nice refactor!

salonishah11

One question otherwise LGTM!

salonishah11 · 2024-09-17T19:33:12Z

...ala/cromwell/backend/google/pipelines/common/PipelinesApiAsyncBackendJobExecutionActor.scala

@@ -134,9 +132,8 @@ class PipelinesApiAsyncBackendJobExecutionActor(override val standardParams: Sta

  override type StandardAsyncRunState = RunStatus

-  override val costHelper: Option[CostPollingHelper[RunStatus]] = Option(new PapiCostPollingHelper(tellMetadata))
-  def statusEquivalentTo(thiz: StandardAsyncRunState)(that: StandardAsyncRunState): Boolean =
-    thiz.toString == that.toString


Isn't .toString needed so that Cromwell doesn't log no-op status changes? I thought this was added recently 🤔

YES, thank you!

jgainerdewar · 2024-09-17T20:39:38Z

I tested an image built off this branch in my BEE and confirmed that the vmStartTime and vmEndTime keys were written to metadata, and that the Bard event shows up in the BigQuery table as expected.

THWiseman added 7 commits September 12, 2024 15:49

initial refactor

db89506

remove startandendtimes

68e8930

rename helper

05f2197

fix up some tests

04c9fbf

fix up tests

16df9c3

comments & format

60e4747

improve test

eb60ff8

THWiseman requested a review from a team as a code owner September 16, 2024 14:02

THWiseman added 5 commits September 16, 2024 10:03

scalafmt

77edf58

Merge branch 'develop' into WX-1792-helper-to-actor

200c472

build issues

219aacb

default to proper kv service

fc45b0a

scalafmt

9d5dd8b

THWiseman requested review from jgainerdewar and salonishah11 September 16, 2024 15:28

jgainerdewar reviewed Sep 16, 2024

View reviewed changes

THWiseman added 3 commits September 17, 2024 13:31

PR feedback

b2b08c0

bug + scalafmt

3b39573

delete unused function

7e87be3

THWiseman commented Sep 17, 2024

View reviewed changes

remove println

d9aa415

jgainerdewar reviewed Sep 17, 2024

View reviewed changes

jgainerdewar approved these changes Sep 17, 2024

View reviewed changes

salonishah11 approved these changes Sep 17, 2024

View reviewed changes

THWiseman added 3 commits September 17, 2024 15:38

fix batch tests

1e445b4

status equality

586f154

delete broken test (i broke it and its OK)

a46c85b

THWiseman merged commit 80fbf59 into develop Sep 18, 2024
38 checks passed

THWiseman deleted the WX-1792-helper-to-actor branch September 18, 2024 12:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WX-1792] Helper to Actor #7544

[WX-1792] Helper to Actor #7544

THWiseman commented Sep 16, 2024 •

edited

Loading

jgainerdewar Sep 16, 2024

jgainerdewar Sep 16, 2024

jgainerdewar Sep 16, 2024

THWiseman Sep 16, 2024 •

edited

Loading

jgainerdewar Sep 16, 2024

THWiseman Sep 17, 2024

THWiseman Sep 17, 2024

jgainerdewar Sep 17, 2024

jgainerdewar left a comment

salonishah11 left a comment

salonishah11 Sep 17, 2024

THWiseman Sep 17, 2024

jgainerdewar commented Sep 17, 2024

[WX-1792] Helper to Actor #7544

[WX-1792] Helper to Actor #7544

Conversation

THWiseman commented Sep 16, 2024 • edited Loading

Description

Release Notes Confirmation

CHANGELOG.md

Terra Release Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

THWiseman Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgainerdewar left a comment

Choose a reason for hiding this comment

salonishah11 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgainerdewar commented Sep 17, 2024

THWiseman commented Sep 16, 2024 •

edited

Loading

`CHANGELOG.md`

THWiseman Sep 16, 2024 •

edited

Loading