refactor!: Update App Concurrency Model #30

jshlbrd · 2022-10-01T01:54:26Z

Description

Refactors the core application's concurrency model to use errgroup
Changes all application errors from public to private
Changes the internal DynamoDB Query API to refer to partition keys as :pk and sort keys as :sk
- This follows best practices for the DynamoDB key condition expression syntax
Standardizes the system's naming conventions and design patterns

Motivation and Context

For the concurrency model refactor, we've known for a while that the existing design has a risk of leaking goroutines when data processing errors happen, but fixing this wasn't a priority because we've never observed it as an issue since the system runs in AWS Lambda (over time the "serverless container" that the application runs in is reaped, cleaning up lingering goroutines).

Now is as good a time as any to try and improve the design, so this PR replaces the Errs and Kill channels with an errgroup and derived context. Functionally this is the same, but provides two advantages for reliability:

we're no longer using buffered channels
errors safely propagate across goroutines

The only significant change in this refactor is the introduction of a Channel struct that allows safer closing of and writing to channels. This was required to interrupt producer goroutines stuck on unbuffered channels when the consumer already terminated, but has an advantage of future proofing changes we might make to channel behavior.

References for these changes:

For the remaining changes, I think they are self-explanatory -- mostly focused on standardizing our approach to conventions across the system with supported documentation.

How Has This Been Tested?

Added a unit test for cmd/app that uses Uber's goleak package to test for goroutine leaks
- Unrelated to testing, cmd/app_test.go contains a fully functional Substation app that can be used as a toy example
Tested on two large, complex deployments

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.

shellcromancer

Nice refactor!

config/config_test.go

shellcromancer · 2022-10-03T15:48:30Z

internal/sink/stdout.go

 		select {
-		case <-kill:
+		case <-ctx.Done():
 			return nil


should this return ctx.Err() as well as the others?

shellcromancer · 2022-10-03T15:54:04Z

process/for_each.go

@@ -11,7 +11,7 @@ import (
 )

 /*
-ForEach processes data by iterating and applying a processor to each element in a JSON array. The processor supports these patterns:
+ForEach processes data by iterating anding a processor to each element in a JSON array. The processor supports these patterns:


nit: grammatical error here

shellcromancer · 2022-10-03T15:54:24Z

process/for_each.go

@@ -44,7 +44,7 @@ type ForEach struct {
 /*
 ForEachOptions contains custom options for the ForEach processor:
 	Processor:
-		processor to apply to the data


nit: grammatical error here

shellcromancer · 2022-10-03T15:56:14Z

process/math.go

@@ -9,7 +9,7 @@ import (
 )

 /*
-Math processes data by applying mathematic operations. The processor supports these patterns:
+Math processes data bying mathematic operations. The processor supports these patterns:


nit: grammatical error here

shellcromancer · 2022-10-03T15:56:27Z

process/pipeline.go


 /*
-Pipeline processes data by applying a series of processors. This processor should be used when data requires complex processing outside of the boundaries of any data structures (see tests for examples). The processor supports these patterns:
+Pipeline processes data bying a series of processors. This processor should be used when data requires complex processing outside of the boundaries of any data structures (see tests for examples). The processor supports these patterns:


nit: grammatical error here

shellcromancer · 2022-10-03T15:56:36Z

process/pipeline.go

@@ -58,7 +58,7 @@ type Pipeline struct {
 /*
 PipelineOptions contains custom options for the Pipeline processor:
 	Processors:
-		array of processors to apply to the data
+		array of processors to to the data


nit: grammatical error here

shellcromancer · 2022-10-03T15:57:19Z

process/pretty_print.go


 /*
-PrettyPrint processes data by applying or reversing prettyprint formatting to JSON.
+PrettyPrint processes data bying or reversing prettyprint formatting to JSON.


nit: grammatical error

julieagnessparks

Looks great! Left a couple comments on typos in documentation but NBD

julieagnessparks · 2022-10-03T15:38:58Z

CONTRIBUTING.md

@@ -2,26 +2,307 @@

 Thank you so much for your interest in contributing to Substation! This document contains guidelines to follow when contributing to the project.

+## Table Of Contents


Wow love the reformatting of the documentation 👏🏻

julieagnessparks · 2022-10-03T15:57:12Z

process/hash.go

@@ -43,7 +45,7 @@ type Hash struct {
 /*
 HashOptions contains custom options for the Hash processor:
 	Algorithm:
-		the hashing algorithm to apply
+		the hashing algorithm to


nit: the hashing algorithm to must be..

julieagnessparks · 2022-10-03T15:58:22Z

process/math.go

@@ -9,7 +9,7 @@ import (
 )

 /*
-Math processes data by applying mathematic operations. The processor supports these patterns:
+Math processes data bying mathematic operations. The processor supports these patterns:


nit: by applying

julieagnessparks · 2022-10-03T15:58:42Z

process/pipeline.go


 /*
-Pipeline processes data by applying a series of processors. This processor should be used when data requires complex processing outside of the boundaries of any data structures (see tests for examples). The processor supports these patterns:
+Pipeline processes data bying a series of processors. This processor should be used when data requires complex processing outside of the boundaries of any data structures (see tests for examples). The processor supports these patterns:


nit: by applying

julieagnessparks · 2022-10-03T15:59:12Z

process/pretty_print.go


 /*
-PrettyPrint processes data by applying or reversing prettyprint formatting to JSON.
+PrettyPrint processes data bying or reversing prettyprint formatting to JSON.
 This processor has significant limitations when used to reverse prettyprint, including:


nit: by applying

julieagnessparks · 2022-10-03T16:00:34Z

process/process.go

@@ -322,7 +319,7 @@ func newBatch(s *[]config.Capsule) []config.Capsule {
 	return make([]config.Capsule, 0, 10)
 }

-// conditionallyApplyBatch uses conditions to dynamically apply processors to a slice of encapsulated data. This is a convenience function for the ApplyBatch method used in most processors.
+// conditionallyApplyBatch uses conditions to dynamically processors to a slice of encapsulated data. This is a convenience function for the ApplyBatch method used in most processors.


nit: to dynamically apply processors

jshlbrd added 30 commits September 12, 2022 02:30

refactor!: standardize dynamodb attributes

ea18e65

Merge branch 'main' into jshlbrd/internal-dynamodb-updates

1cfe892

Merge branch 'main' into jshlbrd/naming-conventions

d939a81

docs: update contrib

5698a90

docs: formatting

ebdedd4

docs: change refs to app

a706e19

docs: README

254a552

docs: fix broken link

7f216b0

test: errgroups

e4e1f0b

test: errgroup file cmd

88444a0

test: channel config

84355e3

refactor: remove unused chan

496bc6b

docs: updated cmd and config

a1111b1

refactor: more errgroup changes

9e75857

docs: update example AWS lambda

a2713ed

refactor: make errs private

7ce011e

docs: update example

f0275a3

refactor: remove unused errs

5b1dfc6

docs: errgroup updates

2dac3f4

docs: contrib update

5eb607e

refactor: minor clean up

13c1eb1

test: add leak detector test

4529ad7

refactor: more cleanup

fafba28

chore: go mod tidy

7a233eb

refactor: cleanup app conventions

1861c76

docs: comment fmt

0f6f8a7

refactor: move concurrency

ea2011a

refactor: cap idiomaticity

ab6a3cd

refactor: processor errs

b1b7f6a

refactor: transform errs

8103a74

jshlbrd added 11 commits October 2, 2022 00:00

refactor: sink errs

f0db2db

refactor: condition errs

de7c9c5

refactor: misc errs

398996a

chore: remove commented tests

e84d154

chore: update examples

1acd561

refactor: misc errs

00e397d

refactor: process errs

527fd69

docs: added design patterns

a4cc24b

docs: remove ref to kill

0680ef1

refactor: err msg

2f6946e

docs: misc updates

f2251e3

jshlbrd marked this pull request as ready for review October 3, 2022 14:56

jshlbrd requested a review from a team as a code owner October 3, 2022 14:56

shellcromancer approved these changes Oct 3, 2022

View reviewed changes

julieagnessparks approved these changes Oct 3, 2022

View reviewed changes

jshlbrd mentioned this pull request Oct 3, 2022

test: Update Tests to Use t.Errorf #31

Closed

fix: nits

1b24246

jshlbrd merged commit d8df4e2 into main Oct 3, 2022

jshlbrd deleted the errgroups branch October 3, 2022 17:37

github-actions bot mentioned this pull request Oct 3, 2022

chore(main): release 0.5.0 #22

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor!: Update App Concurrency Model #30

refactor!: Update App Concurrency Model #30

jshlbrd commented Oct 1, 2022 •

edited

Loading

shellcromancer left a comment

shellcromancer Oct 3, 2022

shellcromancer Oct 3, 2022

shellcromancer Oct 3, 2022

shellcromancer Oct 3, 2022

shellcromancer Oct 3, 2022

shellcromancer Oct 3, 2022

shellcromancer Oct 3, 2022

julieagnessparks left a comment

julieagnessparks Oct 3, 2022

julieagnessparks Oct 3, 2022

julieagnessparks Oct 3, 2022

julieagnessparks Oct 3, 2022

julieagnessparks Oct 3, 2022

julieagnessparks Oct 3, 2022

		@@ -2,26 +2,307 @@

		Thank you so much for your interest in contributing to Substation! This document contains guidelines to follow when contributing to the project.

		## Table Of Contents

refactor!: Update App Concurrency Model #30

refactor!: Update App Concurrency Model #30

Conversation

jshlbrd commented Oct 1, 2022 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

shellcromancer 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

Choose a reason for hiding this comment

julieagnessparks 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

jshlbrd commented Oct 1, 2022 •

edited

Loading