Skip to content

Conversation

@jisantuc
Copy link
Contributor

@jisantuc jisantuc commented Nov 2, 2021

Description

This PR adds a middleware to send any non-HTTPExceptions to application insights. HTTPExceptions are excluded because they're raised for things like 404s, bad auth, etc. (https://fastapi.tiangolo.com/tutorial/handling-errors/), or what you might call normal request failures.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

I raised an error as part of the middleware and was able to see the exception in app insights by providing the staging app insights instrumentation key. I've provided that screenshot in the Azure DevOps task I'm working on.

Checklist:

  • I have performed a self-review
  • Changelog has been updated
  • Unit tests pass locally (./scripts/test)
  • Code is linted and styled (./scripts/format)

@jisantuc
Copy link
Contributor Author

jisantuc commented Nov 4, 2021

@lossyrob does the sku in azm.tf in pc_azm_workspace still need to be a var, or was that a compatibility concern?

@lossyrob
Copy link
Contributor

lossyrob commented Nov 4, 2021

@jisantuc it was a compatibility issue - the former dev environment used the old SKU so it was a var to avoid the update changing the existing instance. We can change remove the var as the new dev stack uses the newer sku, and there's no deployments that use the older sku

@jisantuc
Copy link
Contributor Author

jisantuc commented Nov 10, 2021

I wound up bailing on the liveness path detection efforts. For whatever reason, despite the liveness path configuration appearing to be correct in the deployed service, I was unable to get rid of traces for those requests. However, traces for HEAD requests are still removed, which eliminates a significant source of application insights noise.

@jisantuc jisantuc requested a review from lossyrob November 12, 2021 15:35
@mmcfarland mmcfarland merged commit 837df9e into microsoft:main Dec 15, 2021
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