Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restrict DataDog request instrumentation to /cortex URLs #1076

Merged
merged 1 commit into from
Nov 21, 2023
Merged

Conversation

canac
Copy link
Contributor

@canac canac commented Nov 21, 2023

DataDog's network request instrumentation was causing CORS errors for the designation content requests on branded checkout pages. DataDog was adding headers to the requests, which caused the requests to require a CORS preflight request, which was failing because AEM doesn't implement the OPTIONS method for requests to URLs like https://give.cru.org/content/give/us/en/designations/2/5/9/2/3/2592320.infinity.json. This change restricts instrumentation to cortex gateway URLs.

https://secure.helpscout.net/conversation/2422586630/1052239?folderId=7296729

@canac canac added the On Staging Will be merged to the staging branch by Github Actions label Nov 21, 2023
@canac canac requested review from wrandall22 and dr-bizz November 21, 2023 20:17
@wrandall22
Copy link
Contributor

just FYI @mattdrees ^

@canac canac changed the title Restrict instrumentation to /cortex URLs Restrict DataDog request instrumentation to /cortex URLs Nov 21, 2023
@canac
Copy link
Contributor Author

canac commented Nov 21, 2023

Tested in staging by verifying that running fetch('https://give.cru.org/content/give/us/en/designations/2/5/9/2/3/2592320.infinity.json') doesn't add any additional x-datadog-* headers like https://www.jesusfilm.org/how-to-help/ways-to-donate/give-now-2/ currently does.

@wrandall22
Copy link
Contributor

@canac canac merged commit d7bba09 into master Nov 21, 2023
5 checks passed
@canac canac deleted the hs-1052239 branch November 21, 2023 20:45
@mattdrees
Copy link
Member

Ah, well, that's annoying. I wonder why we didn't get a failure from the Unto branded checkout browser test.

@mattdrees
Copy link
Member

AEM doesn't implement the OPTIONS method

Might be our cloudfront config; I'd find it weird for AEM to not implement OPTIONS. But the solution here is a good one, probably better.

@mattdrees
Copy link
Member

@wrandall22 should we make a synthetic test for jf's branded checkout? I'd wager they're the largest user of that feature.

@wrandall22
Copy link
Contributor

wrandall22 commented Nov 22, 2023

I think there is already a synthetic test for JF branded checkout here.

@wrandall22
Copy link
Contributor

As for the intermittency of this error, I believe it is due to only capturing 10% of sessions. There are a few failures on the JF branded checkout test, but the retry succeeded each time (hence lack of notification).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
On Staging Will be merged to the staging branch by Github Actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants