chore(integrations): oauth2 identity pipeline metrics #79325
Conversation
github-actions
bot
added
the
Scope: Backend
| def exchange_token(self, request: Request, pipeline, code): | ||
| # TODO: this needs the auth yet | ||
| data = self.get_token_params(code=code, redirect_uri=absolute_uri(pipeline.redirect_url())) | ||
| verify_ssl = pipeline.config.get("verify_ssl", True) | ||
| try: | ||
| req = safe_urlopen(self.access_token_url, data=data, verify_ssl=verify_ssl) | ||
| body = safe_urlread(req) | ||
| if req.headers.get("Content-Type", "").startswith("application/x-www-form-urlencoded"): | ||
| return dict(parse_qsl(body)) | ||
| return orjson.loads(body) | ||
| except SSLError: | ||
| logger.info( | ||
| "identity.oauth2.ssl-error", | ||
| extra={"url": self.access_token_url, "verify_ssl": verify_ssl}, | ||
| with record_event( | ||
| IntegrationPipelineViewType.TOKEN_EXCHANGE, pipeline.provider.key |
There was a problem hiding this comment.
even though exchange_token is used inside a function that already has a context manager, i decided to emit another lifecycle metric because the errors are more granular inside exchange_token
| INTEGRATION_PROVIDER_TO_TYPE = { | ||
| v: k for k, values in INTEGRATION_TYPE_TO_PROVIDER.items() for v in values | ||
| } | ||
|
|
There was a problem hiding this comment.
We have some providers that have multiple integration types (namely github), so I think this map will end up incomplete.
sentry/src/sentry/integrations/base.py
Line 153 in 361e5e5
There was a problem hiding this comment.
oof, i wonder if we want to restrict these to the main type? i'm not actually sure what the multiple types are used for here. @ameliahsu
There was a problem hiding this comment.
for the purposes of a metric i'd just want to stick to one, consistently
There was a problem hiding this comment.
right now the types are only really being used for messaging integration onboarding so that we can query OrganizationIntegrationsEndpoint to check if an org has any messaging integrations installed
cathteng
force-pushed
the
cathy/integrations/oauth2-identity-metrics
branch
from
d7da2a2
to
06ff23b
Compare
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #79325 +/- ##
==========================================
- Coverage 78.31% 78.30% -0.01%
==========================================
Files 7140 7140
Lines 315463 315485 +22
Branches 51561 51564 +3
==========================================
+ Hits 247043 247047 +4
- Misses 61953 61963 +10
- Partials 6467 6475 +8 |
| IntegrationProviderSlug.GITHUB, | ||
| IntegrationProviderSlug.GITHUB_ENTERPRISE, | ||
| IntegrationProviderSlug.GITLAB, | ||
| IntegrationProviderSlug.AZURE_DEVOPS, |
There was a problem hiding this comment.
It's worth thinking about this a bit more. It'd be nice to have a centralized place where we can definitively state which integrations have certain meta functionality like this, but only if we can meaningfully keep it in sync with the classes themselves.
This is fine for now though.
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
|
PR reverted: fd5eee0 |
This reverts commit 7cfcaf3. Co-authored-by: cathteng <70817427+cathteng@users.noreply.github.com>
sentry/src/sentry/identity/__init__.py
Lines 25 to 35 in d7da2a2
These are used in the installation flows for:
Missing:
VSTSOAuth2CallbackViewoverwrites theexchange_tokenfunction (VSTSIdentityProvider,VSTSExtensionIdentityProvider)Not applicable
IdentityProvider)