diff --git a/Daily Notes/2024/10/2024-10-22 (Tuesday).md b/Daily Notes/2024/10/2024-10-22 (Tuesday).md index f1b61d4..b4736d7 100644 --- a/Daily Notes/2024/10/2024-10-22 (Tuesday).md +++ b/Daily Notes/2024/10/2024-10-22 (Tuesday).md @@ -14,4 +14,128 @@ A few learnings from launch morning: 6. Slow builds were a major issue 7. No pre-disaster drills was an issue--we couldn't figure out how to deploy from our local machines when we suddenly had to 8. Having Michael Sterling and Bradley jump in and help was vital. Bradley handled the haproxy problems and Michael did a ton of optimization research and implementation + +Feedback from [[Bradley Grainger]] +``` +I haven't really dug into the commerce code much, other than a few places I jumped into to see if there were any quick wins. I want my feedback to be tempered by the knowledge that we were tasked by the business to deliver new commerce features right up until the deadline and really had no time to iron out bugs, run performance tests, etc. That said, I think there are some major architectural challenges in the Commerce code (that perhaps are the results of years of suboptimal decisions compounding on each other): + +- Too much `Task.WhenAll`. This seems to be overused in the backend code. IMO this is a quick band-aid fix for "this route is slow, let's parallelize it". However, under load, it can lead to exponential load on downstream services. (I made 5 parallel calls to service A, that route makes 4 parallel calls to Service B, it makes 6 parallel calls to Service C, you get 120 simultaneous requests to C. This can lead to DoSing ourselves.) Instead, we need to optimize the system as a whole, not an individual route at a time. +- Complex microservice architecture. A trace for ConfigurationWizardSubscriptionsAsync shows (IIRC) a call to OrdersApi which calls into SubscriptionsApi which calls into OrdersApi which calls back into SubscriptionsApi (as well as lots of PaymentPlansApi, ProductsApi, SalesTaxApi, etc.) This could make it complex for individual developers to understand the system as a whole, and may indicate some layering problems. There's also the overhead of all these microservice calls and it becomes more challenging to optimize the system. +- Michael mentioned some RBAR implementations that he saw. We may not be designing and implementing the right abstractions to implement efficient APIs. +- Lots of calls to AccountServices. This has been fixable with Bearer Tokens for a long time; we should do that. + +![+1](https://a.slack-edge.com/production-standard-emoji-assets/14.0/google-small/1f44d.png)![tada](https://a.slack-edge.com/production-standard-emoji-assets/14.0/google-small/1f389.png)![grimacing](https://a.slack-edge.com/production-standard-emoji-assets/14.0/google-small/1f62c.png) + +[10:49](https://faithlife.slack.com/archives/C07TCUA47K3/p1729612154792719) + +Bradley Grainger + +Overall, I think launch went well. We proactively scaled up all services we thought were problematic, and I think they were all provisioned at 2x what they needed to be. But better safe than sorry. +However, we had a blind spot with HAProxy: no one noticed that frontend `maxconn` was only 20,000 which was nowhere near what we needed for the 30k+ people registered for the live event. We also (of course) had the stability issues with keepalived (which probably would have bitten us even if we had increased HAProxy capacity). So it was unfortunate that we _almost_ but not quite got everything scaled up to handle the load. I think we were ![:pinching_hand:](https://a.slack-edge.com/production-standard-emoji-assets/14.0/google-medium/1f90f.png) this close to delivering flawlessly on the day. +``` + +Feedback from [[Michael Sterling]] +``` +Things that went well: + +- We mostly stayed up! +- Our standard Application Insights setup made application performance legible to me, an outsider! +- Commerce **did** have the tools -- per-request caching, per-node caching, product-based caching -- to solve performance problems +- The static page served by HaProxy helped manage incoming traffic +- _You_ Ben were super helpful getting my contributions reviewed & approved + +> we were tasked by the business to deliver new commerce features right up until the deadline + +![:point_up::skin-tone-3:](https://a.slack-edge.com/production-standard-emoji-assets/14.0/google-medium/261d-1f3fc.png) This feels like the big one to me; I was not in the planning process, but I observed signs of "performance/stability didn't get time on the schedule because we filled all available time with features _during the planning process_." + +- In order to go from the "Before" screenshot to the "After" shot in [https://faithlife.atlassian.net/browse/PAY-2935](https://faithlife.atlassian.net/browse/PAY-2935) I didn't use any tools not already available in the Commerce codebase; I just had the luxury of thinking in terms of dependency-calls rather than features when going through the code. +- Nic mentioned something to the effect of not being able to take on stability fixes because he was still working on feature-correctness _day of launch_. +- Another impact of the "gotta get this feature out fast" comes in _how_ we write those last-minute features; take [this change](https://git.faithlife.dev/Logos/Commerce/commit/efac627f2d465528bf6263c65eaf7bce4bae1982#diff-a191907c2a4443ca143d74edacccbbc748dc0d5f0110ca5177552f69a164ae24R229-R271) from last week, which added for each cart: + - A call to fetch the user's owned resources + - _Another_ API call to fetch the subscribable products (which the code had already fetched twice) + - A database call to fetch products by ID (when the code already had fetched the SKUs) to the code-path we had already identified as (a) RBARing rather than batching multiple carts at once and (b) on the "hot path" for launch. + +Suggested takeaways: + +- business buy-in on _saying no to features_ in order to move bug-bash (it felt like) ~two weeks earlier for "performance work" and "correctness fixes." +- Bradley and I volunteered ourselves _anyway_, but we both probably could have come in _earlier_ if asked to; I'm sure there were also developers who could have helped if you put out a call for volunteers to solve well-scoped performance issues. +- Past launches we proactively did some of the load-testing that Bradley initiated +- Get the _business_ to set a SLO and prioritize that as a feature. Pull an SLO out of a hat yourself and make _them_ object to it if they won't give you the commitment. + +> HaProxy KeepAliveD + +This was the primary "failure" if I recall -- with a better "baseline" performance we probably could have served fewer customers the static page, or served them pages faster, but this is the one that took the site down occasionally (and BusinessDesk with it).Suggested takeaways: + +- Definitely make sure that reaching Max Connections doesn't break our health-checks (either in the Azure LB or in KeepAliveD) +- Turn off KeepAliveD? Its _purpose_ is to make sure we can set connection-limits per-node, but.... KeepAliveD has taken us down more often than exceeding per-node connection limits has. In cases where we _really_ need per-node limits (like launch) we can (and did anyway!) manually take down a HaProxy node. +- Investigate Azure Application Gateway instead of our Azure LB + HaProxy "Lift and Shift" setup for apps we do not plan to Container App-ify within ~2 years (?) + +> I'm quite interested in your perspectives on launch day. Specifically, when it comes to the code that Commerce and Payments produced to support it. + +General feedback: + +- Wrangling the build was painful. + - Hugo had to disable the 30-minute flakey Cypress tests for Skeletor + - Suggested fix: [https://faithlife.slack.com/archives/C03R1MQSR51/p1727716783476109?thread_ts=1727711581.537559&cid=C03R1MQSR51](https://faithlife.slack.com/archives/C03R1MQSR51/p1727716783476109?thread_ts=1727711581.537559&cid=C03R1MQSR51) + - Suggested target: no more than **5 minutes** on **all testing** after code is deployed to an environment + - The CEM build spends 8 minutes building the code _for each group_, in order to determine which group has changes. + - This was especially painful when I needed to repeat a build + - The NuGet package restore issue probably goes away when (a) it has less chances to trigger and (b) has fewer builds trying to restore packages all at once + - Suggested fix: do the "build" and "detect which groups have changed" part in _one_ job, which then matrix-jobs out into just triggering tests/deploys for the groups what need it. + - My "master" branch build got stuck waiting behind "PR" builds, and multiple "PR" builds got stuck waiting on each other + - Suggested fix: Independent "PR" sites/environments _for all APIs_ (not just BusinessDesk) so they can all deploy/test independently. + - This could also make it easier to see AppI traces per-PR, which would have helped my cycle-time verifying some of my performance fixes + - The OrdersApi tests took 20-30 minutes + - Suggested target: 5 minutes + - Tribal knowledge: AccountsApi has a _ton_ of test-coverage, and also a ton of strategies to make those tests fast & reliable. + - I accidentally broke something in ProductsApi but didn't catch it until it deployed to test.productsapi and the next build's OrdersApi tests broke + - ProductsApi should have test-coverage on its own corner-cases ideally + - I had to turn off the ImpliedResourcesMap because I didn't want to set up SQLite, and I had to manually set up IIS on my new machine because I didn't want to use Puppet, but these are "me" problems ![:laughing:](https://a.slack-edge.com/production-standard-emoji-assets/14.0/google-medium/1f606.png) +- Institutional aversion to caching + - Example: [here](https://git.faithlife.dev/Logos/Commerce/pull/3362#discussion_r337042) "Last time I checked, getting storefronts from the database was taking ~1ms. I'm not sure it's worth the complexity of adding caching." + - Caching _is hard_ and _does add complexity_ and _can be done poorly_. + - But, there's a chance we've taken cache-avoidance too far and missed some easy wins. + - There also seems to be a "just fetch the data where I need it" approach to a lot of this code, even to the point of not re-using data fetched in a calling method + - We also weren't consistently using _the caching we already had_ -- so we ate the complexity for not the full benefit + - Case Study: We can't _possibly_ need to re-calculate tax as much as we do. But there's no caching whatsoever, not even at SalesTaxApi, and we re-call SalesTaxApi at every level of the Skeletor => OrdersApi => SubscriptionsApi chain. +- "Crochet Code" API surfaces -- augmenting Bradley's point on microservices, because we've got one team owning both ends we've "stitched together" the API surface to a degree that makes reasoning about one end alone impossible + - Example: the `freeTrialOverridePeriodUnitKind ` and `freeTrialOverridePeriodCount` parameters on SubscriptionsApi's `GetSubscribableProductAsync` + - The caller passes up two parameters for the sole purpose of modifying SubscriptionsApi's response in a manner the _caller could have done itself_ even without those parameters + - If `freeTrialOverridePeriodCount` is zero, it disables free trials: [https://git.faithlife.dev/Logos/SubscriptionsApi/blob/master/src/SubscriptionsApi.v1/Services/SubscriptionPreviewData.cs#L253-L257](https://git.faithlife.dev/Logos/SubscriptionsApi/blob/master/src/SubscriptionsApi.v1/Services/SubscriptionPreviewData.cs#L253-L257) + - If set to an amount less than the `desiredContent.SubscribableProduct.Family.MaximumFreeTrialOverridePeriod`, will "Preview" the subscription with that long of a free trial: [https://git.faithlife.dev/Logos/SubscriptionsApi/blob/master/src/SubscriptionsApi.v1/Services/SubscriptionPreviewData.cs#L310-L315](https://git.faithlife.dev/Logos/SubscriptionsApi/blob/master/src/SubscriptionsApi.v1/Services/SubscriptionPreviewData.cs#L310-L315) + - First note: Half of the call-sites _don't even want_ the "Subscription Preview" when fetching a Subscribable Product, but we're doing a lot of work to calculate it for them anyway. The `fields` pattern in Commerce is complicated but can solve this. + - Alternative implementation: if SubscriptionsApi returned a _list_ of potential free-trials up to the maximum `MaximumFreeTrialOverridePeriod`, the caller could _compute for itself_ which one it wanted to use as the "override" + - This decreases the amount of information passed up to SubscriptionsApi, decreasing its testing surface, and making it so more information only flows "one way", making it easier to understand and reason about + - This also eliminates the requirement that calling code to make multiple API calls for each permutation of free-trials it wants to test, which would _halve_ the number of API calls Order Previews needs to make + - This decreases the "coupling" or "crochet stitches" between the two microservices + - It's also essentially "free" for SubscriptionsApi to compute, since it needs no extra "data" (just math) + - Resisting the urge to "just add a parameter" takes strong willpower when one owns both ends of the microservicesand one is tunnel-vision-ing on the _feature_ one is shipping + - Suggested fix: this is hard, maybe an "API Review" process to ensure there is "pain" to changing API surfaces? Make sure there's a high bar for adding query-parameters, e.g., and there's test-coverage for each permutation +- General microservice hygiene: + - Use `fields` more to prevent un-necessary data-fetching + - Ensure data flows "one way" as much as possible + - "Flatten" the call tree (e.g. instead of Skeletor => OrdersApi => SubscriptionsApi => SalesTaxApi, have Skeletor call _each_ of its dependencies _itself_ for the info owned by that dependency then combine them afterwards) + - Reduces the need for "`fields`" as it means less potentially-superfluous data gets fetched from each level + - Eliminate duplicate data-fetching _considering the whole call tree_ starting at the Skeletor root request +``` + +Suggestions from Michael Sterling for improving testing. +``` +For generic flakiness, I'd suggest compiling a list of the flakey ones (by looking at spurious failures across successive runs). Each one has a _reason_ why it's flakey, which _can_ require some skill to divine; but usually it's solvable even with Cypress.For the general test length, I would aim for 5 minutes instead of 18 minutes.Looks like your "Run Cypress Tests" step doesn't put the result files into a GHA Artifact, from which you could download the results for more details on what precisely is slow: [https://git.faithlife.dev/Logos/Skeletor/actions/runs/1034750/job/2100271](https://git.faithlife.dev/Logos/Skeletor/actions/runs/1034750/job/2100271)From the [test results](https://git.faithlife.dev/Logos/Skeletor/runs/2100327#r2), seems like the culprits are: + +- product happy path 1![:white_check_mark:](https://a.slack-edge.com/production-standard-emoji-assets/14.0/google-medium/2705.png) 94s +- billing and shipping checkout 5![:white_check_mark:](https://a.slack-edge.com/production-standard-emoji-assets/14.0/google-medium/2705.png) 98s +- pdp 6![:white_check_mark:](https://a.slack-edge.com/production-standard-emoji-assets/14.0/google-medium/2705.png) 39s +- product drops 39ms +- subscriptions 20![:white_check_mark:](https://a.slack-edge.com/production-standard-emoji-assets/14.0/google-medium/2705.png) 308s + +For each of those test (the flakes and the slow ones), you can make a decision: + +- This test mainly tests UI components; convert it to jest + [record-replay](https://git.faithlife.dev/Logos/FaithlifeEquipment/tree/master/packages/record-replay) for 100% reliability and blazing-fast speed +- This test mainly tests backend C# logic; convert it to a C# HTTP-based integration-test (much more reliable, medium speed) +- This test is truly valuable to verify that the frontend JavaScript logic matches the backend C# logic, and should remain in Cypress. We can speed it up by reducing the scope, we can make it more reliable by improving our selectors and/or wait logic, etc. +- This test covers code we do not change often, and is not worth the time it takes to run _or_ the time it'd take to improve. Delete it. +``` + + # Meetings diff --git a/People/Bradley Grainger.md b/People/Bradley Grainger.md new file mode 100644 index 0000000..e69de29 diff --git a/People/Michael Sterling.md b/People/Michael Sterling.md new file mode 100644 index 0000000..e69de29