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

bootstrapping the xray sdk/middleware #1923

Merged
merged 11 commits into from
Aug 7, 2024
Merged

bootstrapping the xray sdk/middleware #1923

merged 11 commits into from
Aug 7, 2024

Conversation

P0NDER0SA
Copy link
Contributor

@P0NDER0SA P0NDER0SA commented Aug 6, 2024

Summary | Résumé

Just like the title says :)
add the middleware and sdk to admin for aws Xray.

Includes a fix for 500s when it should be a 404 on xxxxxxx/services/wrongendpoint

https://app.zenhub.com/workspaces/notify-planning-core-6411dfb7c95fb80014e0cab0/issues/gh/cds-snc/notification-planning-core/319

Reviewer checklist | Liste de vérification du réviseur

  • This PR does not break existing functionality.
  • This PR does not violate GCNotify's privacy policies.
  • This PR does not raise new security concerns. Refer to our GC Notify Risk Register document on our Google drive.
  • This PR does not significantly alter performance.
  • Additional required documentation resulting of these changes is covered (such as the README, setup instructions, a related ADR or the technical documentation).

⚠ If boxes cannot be checked off before merging the PR, they should be moved to the "Release Instructions" section with appropriate steps required to verify before release. For example, changes to celery code may require tests on staging to verify that performance has not been affected.

Copy link

github-actions bot commented Aug 6, 2024

app/__init__.py Outdated
@@ -699,6 +700,9 @@ def useful_headers_after_request(response):

def register_errorhandlers(application): # noqa (C901 too complex)
def _error_response(error_code):

xray_recorder.begin_segment("error_response")
Copy link
Member

Choose a reason for hiding this comment

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

Would we need to call xray_recorder.end_segment somewhere? The basic usage doc mentions it might have to be called before begin_segment sometimes?

Copy link
Member

Choose a reason for hiding this comment

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

I wonder too if x-ray would detect the error segment automatically with the error response that is generated. Are we testing the segment feature of x-ray here or trying to fix an issue where x-ray fails to detect errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was added as a failsafe for certain errors where the code path results in an empty/null segment. This code will verify that all errors are working as expected and that x-ray is still reporting traces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Stay tuned for code updates

Copy link
Member

@jimleroyer jimleroyer left a comment

Choose a reason for hiding this comment

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

LGTM - Let's also monitor if that slows down our application boot up significantly (i.e. like NewRelic latest updates).

@P0NDER0SA P0NDER0SA merged commit a6f3abb into main Aug 7, 2024
10 checks passed
@P0NDER0SA P0NDER0SA deleted the xray-withfixes branch August 7, 2024 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants