chore(integrations): installation pipeline step metrics #78588

cathteng · 2024-10-03T23:03:36Z

Introduces IntegrationPipelineViewEvent as an EventLifecycleMetric that captures instances of a user going through an integration pipeline view (step). The idea is to wrap each dispatch method of an IntegrationPipelineView class to track how often each step is completed after entering the step.

If failures occur, the newly modified record_failure function can also accept extra data to log, which should help with debugging when customers encounter issues. We currently do not have any logging in the event of a failure during integration installation.

I've added the context manager for GitHub installation in this PR to give an example of how IntegrationPipelineViewEvent would be used.

codecov · 2024-10-07T17:03:48Z

Codecov Report

Attention: Patch coverage is 83.50515% with 16 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/sentry/integrations/github/integration.py	81.33%	10 Missing and 4 partials ⚠️
src/sentry/integrations/utils/metrics.py	90.90%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #78588      +/-   ##
==========================================
+ Coverage   78.21%   78.23%   +0.01%     
==========================================
  Files        7104     7102       -2     
  Lines      312933   313200     +267     
  Branches    51102    51147      +45     
==========================================
+ Hits       244773   245034     +261     
- Misses      61797    61803       +6     
  Partials     6363     6363

GabeVillalobos · 2024-10-08T20:18:15Z

src/sentry/integrations/github/integration.py

+            lifecycle.add_extra(
+                "organization_id",
+                self.active_organization.organization.id if self.active_organization else None,
            )


Not super applicable for this PR I guess, but would it make sense for us to fold extra data like this into the record_event(...).capture() call? Are there cases when we'd want to conditionally add data like this vs just specifying it once at the beginning of the context manager block?

this data is not present at the beginning of the block so i'm adding it in here

GabeVillalobos · 2024-10-08T20:21:36Z

src/sentry/integrations/github/integration.py

+            integration_pending_deletion_exists = False
+            if self.active_organization:
+                # We want to wait until the scheduled deletions finish or else the
+                # post install to migrate repos do not work.
+                integration_pending_deletion_exists = OrganizationIntegration.objects.filter(
+                    integration__provider=GitHubIntegrationProvider.key,
+                    organization_id=self.active_organization.organization.id,
+                    status=ObjectStatus.PENDING_DELETION,
+                ).exists()


The diffs are a bit weird for this change. Is the only difference the context manager and record_failure lifecycle calls?

yes unfortunately the indentation makes it looks like all of this is new. only the context manager, record_failure, add_extra calls are new

It's crazy 😭 I'm surprised github haven't figured out indent yet.

GabeVillalobos · 2024-10-08T20:24:19Z

tests/sentry/integrations/github/test_integration.py

@@ -746,6 +763,8 @@ def test_github_prevent_install_until_pending_deletion_is_complete(self):
            in resp.content
        )

+        self.assert_failure_metric(mock_record, GitHubInstallationError.PENDING_DELETION)


Love to see assertions for this

src/sentry/integrations/utils/metrics.py

sentaur-athena · 2024-10-08T20:24:41Z

src/sentry/integrations/github/integration.py

+    PENDING_DELETION = "GitHub installation pending deletion."
+    INSTALLATION_EXISTS = "Github installed on another Sentry organization."


nit: Are these user facing? If so can we make sure they're sentences with verbs?

these come up in a toaster

src/sentry/integrations/github/integration.py

sentaur-athena · 2024-10-08T21:48:18Z

src/sentry/integrations/github/integration.py

+            integration_pending_deletion_exists = False
+            if self.active_organization:
+                # We want to wait until the scheduled deletions finish or else the
+                # post install to migrate repos do not work.
+                integration_pending_deletion_exists = OrganizationIntegration.objects.filter(
+                    integration__provider=GitHubIntegrationProvider.key,
+                    organization_id=self.active_organization.organization.id,
+                    status=ObjectStatus.PENDING_DELETION,
+                ).exists()


It's crazy 😭 I'm surprised github haven't figured out indent yet.

sentaur-athena · 2024-10-08T21:49:15Z

src/sentry/integrations/github/integration.py

+
+            # At this point, we are past the GitHub "authorize" step
+            if request.GET.get("state") != pipeline.signature:
+                lifecycle.record_failure({"failure_reason": GitHubInstallationError.INVALID_STATE})


What does invalid state mean? Like we're in step 3 without doing step 2 of the pipeline?

i'm not exactly sure... i'm just reusing the definition from before

src/sentry/integrations/github/integration.py

sentaur-athena · 2024-10-08T21:57:54Z

src/sentry/integrations/utils/metrics.py

+    IDENTITY_PROVIDER = "IDENTITY_PROVIDER"
+
+    # GitHub
+    OAUTH_LOGIN = "OAUTH_LOGIN"


Should these have integration name in them? Are they meant to be shared?

we already pass in the integration name when initializing the metric class

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Oct 3, 2024

cathteng changed the title ~~chore(scm): installation step metrics~~ chore(scm): installation pipeline step metrics Oct 3, 2024

cathteng changed the title ~~chore(scm): installation pipeline step metrics~~ chore(scm): github installation pipeline step metrics Oct 3, 2024

vercel bot deployed to Preview October 3, 2024 23:06 View deployment

cathteng added 2 commits October 7, 2024 09:27

installation step metrics

06a5ac7

don't report metrics as SLOs

b6ca552

cathteng force-pushed the cathy/scm/installation-step-metrics branch from bfdff76 to b6ca552 Compare October 7, 2024 16:30

cathteng marked this pull request as ready for review October 7, 2024 16:33

cathteng requested review from a team as code owners October 7, 2024 16:33

cathteng changed the title ~~chore(scm): github installation pipeline step metrics~~ chore(scm): installation pipeline step metrics Oct 7, 2024

vercel bot deployed to Preview October 7, 2024 16:35 View deployment

stringify outcome

9efb337

vercel bot deployed to Preview October 7, 2024 17:53 View deployment

generalize metrics to all integrations

1fbc156

cathteng changed the title ~~chore(scm): installation pipeline step metrics~~ chore(integrations): installation pipeline step metrics Oct 7, 2024

vercel bot deployed to Preview October 7, 2024 23:14 View deployment

add some tests

0043331

vercel bot deployed to Preview October 8, 2024 18:17 View deployment

add self oops

f8c4deb

vercel bot deployed to Preview October 8, 2024 20:14 View deployment

GabeVillalobos reviewed Oct 8, 2024

View reviewed changes

sentaur-athena reviewed Oct 8, 2024

View reviewed changes

nits

6f08b69

cathteng requested review from GabeVillalobos, sentaur-athena and a team October 8, 2024 22:06

vercel bot deployed to Preview October 8, 2024 22:07 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(integrations): installation pipeline step metrics #78588

chore(integrations): installation pipeline step metrics #78588

cathteng commented Oct 3, 2024 •

edited

Loading

codecov bot commented Oct 7, 2024 •

edited

Loading

GabeVillalobos Oct 8, 2024

cathteng Oct 8, 2024

GabeVillalobos Oct 8, 2024

cathteng Oct 8, 2024

sentaur-athena Oct 8, 2024

GabeVillalobos Oct 8, 2024

sentaur-athena Oct 8, 2024

cathteng Oct 8, 2024

sentaur-athena Oct 8, 2024

sentaur-athena Oct 8, 2024

cathteng Oct 8, 2024

sentaur-athena Oct 8, 2024

cathteng Oct 8, 2024

		PENDING_DELETION = "GitHub installation pending deletion."
		INSTALLATION_EXISTS = "Github installed on another Sentry organization."

chore(integrations): installation pipeline step metrics #78588

Are you sure you want to change the base?

chore(integrations): installation pipeline step metrics #78588

Conversation

cathteng commented Oct 3, 2024 • edited Loading

codecov bot commented Oct 7, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cathteng commented Oct 3, 2024 •

edited

Loading

codecov bot commented Oct 7, 2024 •

edited

Loading