feat: propagate a 429 status code if email-provider rate limiting occurs #5801

mzikherman · 2024-06-17T21:55:35Z

This is an alternate implementation of #5798 , different approach altogether.

This is a two-pronged thing:

First - if there are any 429's from an upstream service experienced by an email provider app making a request to MP - we log this via extensions, as emailProviderLimited: true. This is sort-of a place where we temporarily store the fact that this situation has occurred during this request, and some middleware elsewhere in the stack will introspect. The GraphQL spec allows for arbitrary data to live here, and while our middleware will actually introspect and intercept requests with this extension so it never makes it to an end user as-is, I think using extensions for this is reasonable.

Second - we have a middleware on our GraphQL route that is mounted before our regular GraphQL server. This allows it to register a custom res.end function to be able to intercept the response and introspect - if there is extensions and emailProviderLimited: true exists, we can send our custom 429/error response.

This overall still feels wonky to me as there's a bit of hackiness around res.end re-defining, I'm not sure if this is truly better or not than the patch in #5798 - but it is an alternative!

artsyjian

makes sense to me!

joeyAghion

I think I prefer this middleware-like version over patching a lib.

joeyAghion · 2024-06-18T15:37:39Z

src/extensions/email_provider_rate_limit_extension.ts

+          extensions["emailProviderLimited"] = true
+          return true
+        }
+        return false


Do these true/false return values do anything?

it's just the early-returning of the .some iterator. I can double-check this snippet (it may or may not have been AI-generated).

Can we use something other than some here? To joey's point, return true / false means that we should / should not return a result from some to be collected into a variable. If we're simply iterating over an array to check for values to be merged into an object, we can use forEach, which doesn't return anything and reads more correctly.

Something like:

const extensions = result.errors? .flatMap(flattenErrors) .reduce((acc, error) => { const httpStatusCode = statusCodeForError(error) if (httpStatusCode === 429) { acc.emailProviderLimited = true } return acc }, {}) return extensions

joeyAghion · 2024-06-18T15:40:46Z

src/index.ts

+    const contentType = res.get("content-type") || "text/html"
+
+    if (contentType.indexOf("application/json") === 0) {
+      const response = JSON.parse(chunk)


It's unfortunate we have to re-parse this.

Uninformed question: I've seen context used for passing request data around. Is it not possible to use that to make the 429 status available, as opposed to rendering it into the extensions response?

I suppose one could use context for that, but that's a GraphQL-server-concept (as in, within the scope of the special express-graphql sub-app mounted at our /v2 endpoint) - and so this middleware (which is a totally separate Express middleware mounted before that sub-app) doesn't have access to that context.

In general trying to implement some version of this was tricky for exactly this reason!

Can't do it natively with express-graphql as there's no parseResponse or any other 'hook' available -> leading to possible patch.

Can't do it natively with Express middleware as that won't have access to the completed response, unless there's some tricksy res.on("end", ... overriding - and then this approach implies that we do need to store the 'this is an email provider request that resulted in rate limiting' fact in the response somewhere.

joeyAghion · 2024-06-18T15:42:11Z

src/index.ts

+      const response = JSON.parse(chunk)
+
+      if (response.extensions && response.extensions.emailProviderLimited) {
+        res.status(429).json({ error: "rate limit reached, try again later" })


This drops the other response data (including the extensions data), and just returns this tiny error blob, right?

joeyAghion · 2024-06-18T15:45:08Z

src/index.ts

@@ -84,10 +85,17 @@ function createExtensions(document, result, requestID, userAgent) {
    ? fetchLoggerRequestDone(requestID, userAgent)
    : {}

+  // Should this introspect xapp token/roles for `email_provider` instead?


I wondered this same thing. It would be more reliable, but more complex code-wise. Overall I think it's OK to use the user-agent in this way because Braze is a well-behaved integration. The worst case is that a caller spoofing their user-agent gets this same, conservative treatment.

Exactly, worst-case winds up being this is applied more often/overzealously. I think it's fine that we try to call it 'email provider' in our code, but somewhere in the stack (ideally maybe kept to one or minimal spots), there is a check that winds up having to mention 'braze'.

You may not know and this is a bit of a off topic question but I've wondered for a while about the ID thats part of Braze user-agent:

Braze Sender 954a8af2d130500d09e736a5e8ce1db59da5c25f; Metaphysics

What is the purpose of that ID? Is that something that is unique for the organization that Braze has a relationship with or perhaps its masking information to make it harder to spoof the user-agent? Does the ID stay constant or can it change at any point? 🤔

mzikherman · 2024-06-18T18:03:53Z

I actually think this can be merged independently of any backend work we might do to add rate limiting, and more as just an enhancement within Metaphysics - since we know we want this behavior - in fact maybe we even want this behavior today in case Braze, via Metaphysics, is already being rate-limited by a backend app.

src/index.ts

ovasdi

I am not sure either, as far as which approach is better, but by my understanding this alternative approach would continue to work even if at some future point express-graphql is removed from the codebase. Assuming this is true, this gets a +1 from me over the other approach.

damassi · 2024-07-04T00:03:26Z

src/extensions/email_provider_rate_limit_extension.ts

@@ -0,0 +1,22 @@
+import { flattenErrors, statusCodeForError } from "lib/graphqlErrorHandler"


Minor, but can we rename this file to emailProviderRateLimitExtension so that it matches idiomatic JS? We have snake_case in MP, but that's very old.

damassi · 2024-07-04T00:04:29Z

src/extensions/email_provider_rate_limit_extension.ts

+
+// Log in `extensions` if this request was by an email provider and resulted
+// in a 429 rate limit error from a downstream request made using `fetch`.
+export const emailProviderRateLimitExtension = (_documentAST, result) => {


Ie, with the named export here, and the file name being snake case, that creates a 1:2 naming representation in the codebase, which is unnecessary.

damassi · 2024-07-04T00:16:13Z

src/index.ts

+// rate limting occurred.
+//
+// If so, we return a 429 status code and a JSON error message.
+app.use("/v2", function (req, res, next) {


If we're gonna chatGPT stuff we should at least follow modern JS conventions and audit the output 😄 we never see these types of anonymous function declarations anywhere, not for years. Please use arrow functions

damassi · 2024-07-04T00:16:28Z

src/index.ts

+
+  const originalEnd = res.end
+
+  res.end = function (...args) {


feat: propagate a 429 status code if email-provider rate limiting occurs

31fae9d

mzikherman requested review from joeyAghion, cavvia, damassi, ovasdi, brainbicycle, artsyjian and a team June 17, 2024 21:55

mzikherman self-assigned this Jun 17, 2024

mzikherman force-pushed the try-middleware branch from 5c3260e to 31fae9d Compare June 17, 2024 21:55

mzikherman mentioned this pull request Jun 17, 2024

feat: propagate a 429 status code if Braze rate limiting occurs #5798

Open

artsyjian approved these changes Jun 18, 2024

View reviewed changes

joeyAghion approved these changes Jun 18, 2024

View reviewed changes

brainbicycle approved these changes Jun 18, 2024

View reviewed changes

src/index.ts Show resolved Hide resolved

ovasdi approved these changes Jun 20, 2024

View reviewed changes

damassi reviewed Jul 4, 2024

View reviewed changes

src/index.ts

const originalEnd = res.end

res.end = function (...args) {

Copy link

Member

damassi Jul 4, 2024

Choose a reason for hiding this comment

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

ditto

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: propagate a 429 status code if email-provider rate limiting occurs #5801

feat: propagate a 429 status code if email-provider rate limiting occurs #5801

mzikherman commented Jun 17, 2024

artsyjian left a comment

joeyAghion left a comment

joeyAghion Jun 18, 2024

mzikherman Jun 18, 2024

damassi Jul 4, 2024

damassi Jul 4, 2024

joeyAghion Jun 18, 2024

mzikherman Jun 18, 2024

joeyAghion Jun 18, 2024

mzikherman Jun 18, 2024

joeyAghion Jun 18, 2024

mzikherman Jun 18, 2024

ovasdi Jun 20, 2024

mzikherman commented Jun 18, 2024

ovasdi left a comment

damassi Jul 4, 2024

damassi Jul 4, 2024

damassi Jul 4, 2024 •

edited

Loading

damassi Jul 4, 2024

		@@ -0,0 +1,22 @@
		import { flattenErrors, statusCodeForError } from "lib/graphqlErrorHandler"

feat: propagate a 429 status code if email-provider rate limiting occurs #5801

Are you sure you want to change the base?

feat: propagate a 429 status code if email-provider rate limiting occurs #5801

Conversation

mzikherman commented Jun 17, 2024

artsyjian left a comment

Choose a reason for hiding this comment

joeyAghion 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

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

mzikherman commented Jun 18, 2024

ovasdi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damassi Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damassi Jul 4, 2024 •

edited

Loading