Conversation
dschom
force-pushed
the
FXA-13177
branch
4 times, most recently
from
fb236e0 to
e6273ee
Compare
| nodeTracingInitialized = !!initTracing(config.tracing, log); | ||
| } | ||
|
|
||
| // Important! Order matters here. If otel is configured, this shoudl be done after OTEL initialization! |
There was a problem hiding this comment.
Lots of important things in here 😅
There was a problem hiding this comment.
Yep, turns out there's a couple ways to shoot yourself in the foot here... Don't ask how I'd know. 😅
| eventFilters: [filterSentryEvent], | ||
| integrations: [Sentry.linkedErrorsIntegration({ key: 'jse_cause' })], | ||
| integrations: [ | ||
| // Important! This fixes a ton of problems with our previous integration. |
| //*/ | ||
|
|
||
| Sentry.setExtra('request_payload', request.app?.acceptLanguage || {}); | ||
| Sentry.setExtra('request_payload', request.payload || {}); |
There was a problem hiding this comment.
This could potentially log authPW or other sensitive user data?
There was a problem hiding this comment.
Yes, however when testing this it appeared to get filtered out over on Sentry's side by filtering settings there. I'd be open to redacting certain fields though. I can't see our org wide policies, and someone could change them at any time, so you are probably right that doing this in code is safer.
|
|
||
| Sentry.setExtra('request_payload', request.app?.acceptLanguage || {}); | ||
| Sentry.setExtra('request_payload', request.payload || {}); | ||
| Sentry.setExtra('request_headers', request.headers || {}); |
There was a problem hiding this comment.
Same idea here, we could log the authorization header.
There was a problem hiding this comment.
@vbudhram I just included a filtering function.
|
Finally got Sentry setup, events do come in with your STR 
|
Because: - We would see context from other errors cross pollinate - In auth server error capture was particularly poor This Commit: - Removes old cruft that is no longer needed with sentry 10 - Now that Sentry uses, OTEL under the hood makes necessary adjustments to our OTEL initialization - Adds handler for testing / validating this works - Updates logic to ensure proper initialization of both OTEL and Sentry. - Makes adjustments to libs/shared to keep things aligned - Stops writing requests to error object. This now happens via the integration. - Sets the user state on the error
2 participants
Because
This pull request
Issue that this pull request solves
Closes: FXA-13177
Checklist
Put an
xin the boxes that applyScreenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
Testing is largely exploratory. Here's some tips
Add this to your .env, run
dotenv -- yarn startThen run
seq 1 15 | xargs -n1 -P5 -I{} curl -s http://localhost:9000/v1/boomand check Sentry, with a 'local' env filter. You might have wait a minute, but you will see the capture, and you'll notice the traces are isolated again.