CUMULUS-260 update pdr stats #235

jennyhliu · 2018-03-06T20:41:35Z

Summary: Summary of changes

Addresses CUMULUS-260: PDR page on dashboard only shows zero

Currently the pdr status stays in 'running' even if the granules are processed and completed.

Changes

Update lambda pdr-status-check to take pdr as additional input and output playload, so that pdr stats (granule stats in the pdr) can be updated by the cumulus api when the workflow is complete.
Add lambda function sf-sns-report which is copied from @cumulus/api/lambdas/sf-sns-broadcase with modification, sf-sns-report can be used to report step function status anywhere inside a step function. So add step sf-sns-report after each pdr-status-check, we will get the PDR status progress at real time.
Update pdr-status-check to handle ExecutionDoesNotExist error.
Fixed @cumulus/common/aws setGranuleStatus

Test Plan

Things that should succeed before merging.

Unit tests - updated
Adhoc testing - tested in aws

Reviewers: @scisco @laurenfrederick

jennyhliu · 2018-03-08T14:33:49Z

Actually, we can call indexer.partialRecordUpdate to update the pdr.stats only from pdr-status-check. I will try that

scisco

Could you add changelog here?

… pdrstats

…to pdrstats

abarciauskas-bgse · 2018-03-13T13:34:28Z

CHANGELOG.md

+- **CUMULUS-260: "PDR page on dashboard only shows zeros."** The PDR stats in LPDAAC are all 0s, even if the dashboard has been fixed to retrieve the correct fields.  The current version of pdr-status-check has a few issues.
+  - pdr is not included in the input/output schema.  It's available from the input event.  So the pdr status and stats are not updated when the ParsePdr workflow is complete.  Adding the pdr to the input/output of the task will fix this.
+  - pdr-status-check doesn't update pdr stats which prevent the real time pdr progress from showing up in the dashboard. To solve this, added lambda function sf-sns-report which is copied from @cumulus/api/lambdas/sf-sns-broadcase with modification, sf-sns-report can be used to report step function status anywhere inside a step function.  So add step sf-sns-report after each pdr-status-check, we will get the PDR status progress at real time.
+  - It's possible a execution is still in the queue and doesn't exist in sfn yet.  Added code to handle 'ExecutionDoesNotExist' error when checking the execution status.


minor grammar fix: It's possible an execution is still in the queue

abarciauskas-bgse · 2018-03-13T13:40:39Z

cumulus/tasks/pdr-status-check/index.js

+    try {
+      const execution = await aws.sfn().describeExecution({ executionArn }).promise();
+      executions.push(execution);
+    }


nit: should } catch (e) { should be on one line

eslint doesn't like the one-line style

weird that seems inconsistent with https://github.com/airbnb/javascript#blocks--cuddled-elses

That's because of this override: https://github.com/cumulus-nasa/cumulus/blob/master/.eslintrc.json#L86.

Our eslintrc is requiring the "stroustrup" style for braces: https://eslint.org/docs/rules/brace-style

abarciauskas-bgse · 2018-03-13T13:41:38Z

cumulus/tasks/pdr-status-check/index.js

+      // it's ok if a execution is still in the queue and has not be executed
+      if (e.code === 'ExecutionDoesNotExist') {
+        executions.push({ executionArn: executionArn, status: 'RUNNING' });
+      }


Same thing here

} else { throw e; }

yeah, our eslint doesn't like that.

Same comment as the other instance:

That's because of this override: https://github.com/cumulus-nasa/cumulus/blob/master/.eslintrc.json#L86.

Our eslintrc is requiring the "stroustrup" style for braces: https://eslint.org/docs/rules/brace-style

abarciauskas-bgse · 2018-03-13T13:54:04Z

cumulus/tasks/sf-sns-report/index.js

+  }
+  // Error and error keys are not part of the cumulus message
+  // and if they appear in the message something is seriously wrong
+  else if (event.Error || event.error) {


Do we know an instance of this occurring? A better option might be to whitelist the top-level keys we expect from a cumulus message, otherwise this seems unnecessary

I don't. I don't know if removing this logic will break something.

this was reported in an earlier bug report and this is the fix for that bug.

ah ok - do you recall if that bug was before or after cumulus message adapter integration?

@abarciauskas-bgse brings up a very good point. When you're using the message adapter, the only keys that the simplified event is going to have are "config" and "input". This is never going to trigger.

abarciauskas-bgse · 2018-03-13T13:54:28Z

cumulus/tasks/sf-sns-report/index.js

+ * @returns {boolean} true if there was an exception, false otherwise
+ */
+function eventFailed(event) {
+  if (event.exception) {


Maybe flatten these nested conditionals?

if (event.exception && typeof(event.exception) === 'object' && Object.keys(event.exception.length) > 0) { return true }

yes. this is neat

abarciauskas-bgse · 2018-03-13T13:55:33Z

cumulus/tasks/sf-sns-report/index.js

+
+/**
+ * if the cumulus message shows that a previous step failed,
+ * this function extract the error message from the cumulus message


this function extracts the error message

abarciauskas-bgse · 2018-03-13T13:56:07Z

cumulus/tasks/sf-sns-report/index.js

+/**
+ * if the cumulus message shows that a previous step failed,
+ * this function extract the error message from the cumulus message
+ * and fail the function with that information. This ensures that the


and fails the function

abarciauskas-bgse · 2018-03-13T13:58:58Z

cumulus/tasks/sf-sns-report/index.js

+ * @returns {Promise} updated event object
+ */
+function handler(event, context, callback) {
+  return StepFunction.pullEvent(event).then((message) =>


nit: wrap in curly brace:

return StepFunction.pullEvent(event).then((message) => { cumulusMessageAdapter.runCumulusTask(publishSnsMessage, message, context, callback)); }

abarciauskas-bgse · 2018-03-13T14:00:36Z

cumulus/tasks/sf-sns-report/tests/index.js

+});
+
+
+test('send report when sfn is finished and granule succeed', async (t) => {


send report when sfn is finished and granule has succeeded

abarciauskas-bgse · 2018-03-13T14:37:07Z

@jennyhliu you mentioned in the PR description Add lambda function sf-sns-report which is copied from /lambdas/sf-sns-broadcase with modification should some of the shared functionality be pulled into a separate package which becomes a dependency of both to reduce duplication of code?

scisco

asked for a small change

scisco · 2018-03-13T13:40:15Z

cumulus/tasks/pdr-status-check/package.json

@@ -40,6 +40,7 @@
    ]
  },
  "dependencies": {
+    "@cumulus/api": "^1.1.1",


This is not used any where. Please remove it

scisco · 2018-03-13T13:41:13Z

cumulus/tasks/sf-sns-report/README.md

@@ -0,0 +1,55 @@
+# @cumulus/queue-pdrs


The name is incorrect

scisco · 2018-03-13T14:43:36Z

@abarciauskas-bgse I think we should retire that other lambda function in the api package. But retiring it will break existing deployments. so we should come up with a plan for that.

…to pdrstats

jennyhliu · 2018-03-13T16:44:48Z

@abarciauskas-bgse @scisco updated based on comments.

marchuffnagle · 2018-03-14T13:40:39Z

packages/common/aws.js

-  await exports.s3().putObject(bucket, key, '', null, { executionArn, status }).promise();
+  const params = { Bucket: bucket, Key: key };
+  params.Metadata = { executionArn, status };
+  await exports.s3().putObject(params).promise();


I've filed a ticket to take a look at this functionality: https://bugs.earthdata.nasa.gov/browse/CUMULUS-420

marchuffnagle · 2018-03-14T17:39:30Z

CHANGELOG.md

@@ -17,6 +17,11 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
  - `@cumulus/deployment` deploys DynamoDB streams for the Collections, Providers and Rules tables as well as a new lambda function called `dbIndexer`. The `dbIndexer` lambda has an event source mapping which listens to each of the DynamoDB streams. The dbIndexer lambda receives events referencing operations on the DynamoDB table and updates the elasticsearch cluster accordingly.
  - The `@cumulus/api` endpoints for collections, providers and rules _only_ query DynamoDB, with the exception of LIST endpoints and the collections' GET endpoint.

+- **CUMULUS-260: "PDR page on dashboard only shows zeros."** The PDR stats in LPDAAC are all 0s, even if the dashboard has been fixed to retrieve the correct fields.  The current version of pdr-status-check has a few issues.
+  - pdr is not included in the input/output schema.  It's available from the input event.  So the pdr status and stats are not updated when the ParsePdr workflow is complete.  Adding the pdr to the input/output of the task will fix this.
+  - pdr-status-check doesn't update pdr stats which prevent the real time pdr progress from showing up in the dashboard. To solve this, added lambda function sf-sns-report which is copied from @cumulus/api/lambdas/sf-sns-broadcase with modification, sf-sns-report can be used to report step function status anywhere inside a step function.  So add step sf-sns-report after each pdr-status-check, we will get the PDR status progress at real time.


"broadcast", not "broadcase"

marchuffnagle · 2018-03-14T17:50:28Z

cumulus/tasks/sf-sns-report/index.js

+function eventFailed(event) {
+  // event has exception
+  // and it is needed to avoid flagging cases like "exception: {}" or "exception: 'none'"
+  if (event.exception && (typeof event.exception === 'object') &&


The first check for event.exception is not necessary. If it isn't set, then (typeof event.exception === 'object') will be false. Can just be:

function eventFailed(event) { // event has exception // and it is needed to avoid flagging cases like "exception: {}" or "exception: 'none'" if ((typeof event.exception === 'object') && (Object.keys(event.exception).length > 0)) return true; // Error and error keys are not part of the cumulus message // and if they appear in the message something is seriously wrong if (event.Error || event.error) return true; return false; }

This example is also formatted so it's a little easier to read.

actually ``(event.exception)check is need to take care of the caseevent.exception=null`, otherwise `Object.keys(event.exception)` will throw exception.

marchuffnagle · 2018-03-14T18:07:55Z

cumulus/tasks/sf-sns-report/index.js

+ * @param {Object} event - aws event object
+ * @returns {undefined} throws an error and does not return anything
+ */
+function makeLambdaFunctionFail(event) {


It's a little hard to follow what this function is doing. It might be easier with something like:

function buildError(type, cause) { let ErrorClass; if (Object.keys(errors).includes(type)) ErrorClass = errors[type]; else if (type === 'TypeError') ErrorClass = TypeError; else ErrorClass = Error; return new ErrorClass(cause); } function makeLambdaFunctionFail(event) { const error = event.exception || event.error; if (!error) throw new Error('Step Function failed for an unknown reason.'); throw buildError(error.Error, error.Cause); }

Thanks. This makes the code much clear. Sorry I didn't try to understand the code.

marchuffnagle · 2018-03-14T18:10:54Z

cumulus/tasks/sf-sns-report/index.js

+ * @returns {Promise} updated event object
+ */
+function handler(event, context, callback) {
+  return StepFunction.pullEvent(event).then((message) => {


Pulling the event is handled by the message adapter and should not be required here.

marchuffnagle · 2018-03-14T18:11:55Z

cumulus/tasks/sf-sns-report/schemas/config.json

+  "description": "Describes the config used by the sf-sns-report task",
+  "type": "object",
+  "additionalProperties": false,
+  "properties": {


I assume that all of these properties are required, so they should be marked as required here.

Based on the code, they are only needed when the task is the last step of the step function and a granule state is set.

marchuffnagle · 2018-03-14T18:13:33Z

cumulus/tasks/sf-sns-report/index.js

+ * schema that is passed to the next task in the workflow
+ */
+async function publishSnsMessage(event) {
+  const config = get(event, 'config', []);


The config is an Object, not an Array. That being said, message adapter will ensure that config is always set, so there's no need for a default value.

marchuffnagle · 2018-03-14T18:13:51Z

cumulus/tasks/sf-sns-report/index.js

+ */
+async function publishSnsMessage(event) {
+  const config = get(event, 'config', []);
+  const message = get(event, 'input', []);


The input is an Object, not an Array. That being said, message adapter will ensure that config is always set, so there's no need for a default value.

marchuffnagle · 2018-03-14T18:20:11Z

cumulus/tasks/sf-sns-report/index.js

+      if (granuleId) {
+        await setGranuleStatus(
+          granuleId,
+          get(config, 'stack', null),


If any of these values are not set, you're passing null in their place. What is setGranuleStatus going to do with a null bucket, or any of the other parameters for that matter?

Updated the code and schema, there is no good way to prevent the user from misconfiguring the parameters.

marchuffnagle · 2018-03-14T18:26:04Z

cumulus/tasks/sf-sns-report/tests/index.js

@@ -0,0 +1,87 @@
+'use strict';


None of these tests are verifying that the correct message was sent to SNS. They are only looking at the return value from the function. Localstack has SNS support, please update the tests to verify that the correct messages are being sent to SNS.

Updated the function to return the sns response. If the MessageId is returned, then the message is sent to SNS. There is no easy way to retrieve the message sent to SNS.

marchuffnagle

I've included a number of comments that need to be addressed before this can be merged.

scisco · 2018-03-14T21:15:01Z

cumulus/tasks/sf-sns-report/index.js

+    makeLambdaFunctionFail(message);
+  }
+
+  return message;


@jennyhliu I think we should NOT return the whole message here. If the user forgets to include the outputs key in the cumulus config, the message ends up getting larger and larger.

Could you instead just return the output of the sns aws call?

Updated the code to return the output from sns response, and the README.md

…to pdrstats

jennyhliu · 2018-03-15T12:55:40Z

@scisco @yjpa7145 made updates based on comments. please review

…to pdrstats

marchuffnagle · 2018-03-15T13:22:10Z

cumulus/tasks/sf-sns-report/index.js

+      if (granuleId) {
+        await setGranuleStatus(
+          granuleId,
+          get(config, 'stack'),


Because you aren't looking deep into an object, there is no need for get() here. Simply saying config.stack accomplishes the same thing. The same goes for the next few lines as well.

scisco

Looks good to me 👏

marchuffnagle · 2018-03-15T13:27:00Z

cumulus/tasks/sf-sns-report/schemas/config.json

+  },
+  "oneOf": [
+    {
+      "required": ["stack", "bucket", "stateMachine", "executionTime"]


stack, bucket, and stateMachine should always be required. Should executionTime actually be executionName? I don't see executionTime anywhere in the code, but I do see executionName.

…to pdrstats

marchuffnagle

The sf-sns-report task needs to have an input.json schema file defined.

scisco · 2018-03-15T14:07:38Z

@jennyhliu the input.json should accept any object. Basically whatever you pass to the task it will publish to sns.

The concensus is that this all looks good.

update pdr stats

e574755

jennyhliu requested review from scisco and laurenfrederick March 7, 2018 17:10

scisco suggested changes Mar 8, 2018

View reviewed changes

jennyhliu and others added 7 commits March 9, 2018 09:28

Merge branch 'master' of https://github.com/cumulus-nasa/cumulus into…

c796810

… pdrstats

update pdr stats

ca5634d

remove update pdr stats from pdr-status-check

ff23e5d

use cumulus-message-adapter

1f3a8ab

Merge branch 'master' into pdrstats

76cad35

update readme

cd14fd0

Merge branch 'pdrstats' of https://github.com/cumulus-nasa/cumulus in…

1601eca

…to pdrstats

abarciauskas-bgse reviewed Mar 13, 2018

View reviewed changes

jennyhliu requested a review from marchuffnagle March 13, 2018 13:34

Merge branch 'master' into pdrstats

091e9d4

jennyhliu changed the title ~~WIP CUMULUS-260 update pdr stats~~ CUMULUS-260 update pdr stats Mar 13, 2018

abarciauskas-bgse reviewed Mar 13, 2018

View reviewed changes

scisco suggested changes Mar 13, 2018

View reviewed changes

jennyhliu added 2 commits March 13, 2018 11:46

fixes

d939030

Merge branch 'pdrstats' of https://github.com/cumulus-nasa/cumulus in…

853c835

…to pdrstats

marchuffnagle reviewed Mar 14, 2018

View reviewed changes

marchuffnagle suggested changes Mar 14, 2018

View reviewed changes

Merge branch 'master' into pdrstats

a835a3e

scisco reviewed Mar 14, 2018

View reviewed changes

Alireza and others added 3 commits March 14, 2018 17:32

Merge branch 'master' into pdrstats

2ab76f6

more fixes

b2507ce

Merge branch 'pdrstats' of https://github.com/cumulus-nasa/cumulus in…

68bc62e

…to pdrstats

Merge branch 'pdrstats' of https://github.com/cumulus-nasa/cumulus in…

4b49dcd

…to pdrstats

marchuffnagle reviewed Mar 15, 2018

View reviewed changes

scisco approved these changes Mar 15, 2018

View reviewed changes

marchuffnagle reviewed Mar 15, 2018

View reviewed changes

jennyhliu added 2 commits March 15, 2018 09:36

more fix

a9c446d

Merge branch 'pdrstats' of https://github.com/cumulus-nasa/cumulus in…

d51ed64

…to pdrstats

marchuffnagle previously requested changes Mar 15, 2018

View reviewed changes

jennyhliu added 2 commits March 15, 2018 10:41

Merge branch 'master' into pdrstats

7307d5f

Merge branch 'master' into pdrstats

d672146

jennyhliu merged commit 449c0fb into master Mar 15, 2018

scisco deleted the pdrstats branch March 15, 2018 15:47

		});


		test('send report when sfn is finished and granule succeed', async (t) => {

CUMULUS-260 update pdr stats #235

CUMULUS-260 update pdr stats #235

Conversation

jennyhliu commented Mar 6, 2018 • edited Loading

Changes

Test Plan

jennyhliu commented Mar 8, 2018

scisco left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abarciauskas-bgse Mar 13, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marchuffnagle Mar 15, 2018 • 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

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

Choose a reason for hiding this comment

abarciauskas-bgse commented Mar 13, 2018

scisco left a 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

Choose a reason for hiding this comment

scisco commented Mar 13, 2018

jennyhliu commented Mar 13, 2018

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marchuffnagle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennyhliu commented Mar 15, 2018

Choose a reason for hiding this comment

scisco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marchuffnagle left a comment

Choose a reason for hiding this comment

scisco commented Mar 15, 2018

jennyhliu commented Mar 6, 2018 •

edited

Loading

abarciauskas-bgse Mar 13, 2018 •

edited

Loading

marchuffnagle Mar 15, 2018 •

edited

Loading