feat(ecosystem): Adds Project Management integration SLOs #80719

GabeVillalobos · 2024-11-14T00:28:34Z

WIP - Adds Project Management/Issues SLO metrics

This PR adds metric coverage to the majority of Project Management features:

Issue Creation (including alert rules) - src/sentry/api/endpoints/group_integration_details.py
Outbound status syncing - src/sentry/integrations/tasks/sync_status_outbound.py
Outbound assignment syncing - src/sentry/integrations/tasks/sync_assignee_outbound.py
Issue linking - src/sentry/api/endpoints/group_integration_details.py
Inbound assignment syncing
Installation metrics for ticketing integrations

Has Metrics, Needs Testing:

Inbound status syncing

Incidental Work:

Adds new tests for sync_outbound_assignment and sync_outbound_status tasks, which previously only had endpoint level testing.
Adds new test for the shared sync_group_assignee_inbound utility (src/sentry/integrations/utils/sync.py)

GabeVillalobos · 2024-11-15T19:45:52Z

src/sentry/api/endpoints/group_integration_details.py

+                return Response(exc.field_errors, status=400)
+            except IntegrationError as e:
+                lifecycle.record_halt(e)
+                return Response({"non_field_errors": [str(e)]}, status=400)


To fix the problem, we should avoid exposing the detailed exception message to the end user. Instead, we can log the detailed error message on the server for debugging purposes and return a generic error message to the user. This approach ensures that sensitive information is not exposed while still allowing developers to diagnose issues.

Modify the exception handling code to log the detailed error message.

Return a generic error message to the user instead of the detailed exception message.

This isn't new code, this is what allows errors to propagate up to the UI when ticket creation or linking fails on the integrator side (usually a config issue with the ticket contents)

src/sentry/api/endpoints/group_integration_details.py

+                return Response(exc.field_errors, status=400)
+            except IntegrationError as e:
+                lifecycle.record_halt(e)
+                return Response({"non_field_errors": [str(e)]}, status=400)


To fix the problem, we need to ensure that detailed exception messages are not exposed to the end user. Instead, we should log the detailed error message on the server and return a generic error message to the user. This can be achieved by modifying the exception handling code to log the exception and return a generic error message.

Import the logging module to log the detailed error message.

Modify the exception handling blocks to log the exception and return a generic error message.

GabeVillalobos · 2024-11-14T00:34:11Z

src/sentry/integrations/project_management/metrics.py

+@dataclass
+class ProjectManagementEvent(IntegrationEventLifecycleMetric):
+    action_type: ProjectManagementActionType
+    integration: Integration | RpcIntegration


Turns out we don't always have access to integration installations (specifically the inbound assignment code), so I made this use integration/rpc integration.

GabeVillalobos · 2024-11-14T00:36:03Z

src/sentry/rules/actions/integrations/create_ticket/utils.py

+                logger.info(
+                    "%s.rule_trigger.create_ticket.failure",
+                    provider,
+                    extra={
+                        "rule_id": rule_id,
+                        "provider": provider,
+                        "integration_id": integration.id,
+                        "error_message": str(e),
+                        "exception_type": type(e).__name__,
+                    },


It's nice being able to delete these logs in the future

GabeVillalobos · 2024-11-14T00:36:48Z

tests/sentry/integrations/jira/test_ticket_action.py

+class BrokenJiraMock(MockJira):
+    """
+    Subclass of MockJira client, which implements a broken create_issue method
+    to simulate a misconfigured alert rule for Jira.
+    """
+
+    def create_issue(self, raw_form_data):
+        raise ApiInvalidRequestError("Invalid data entered")


This is gross, but was the easiest way to trigger a failure scenario.

codecov · 2024-11-14T01:02:52Z

Codecov Report

Attention: Patch coverage is 93.75000% with 10 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/sentry/integrations/jira/utils/api.py	71.42%	5 Missing and 1 partial ⚠️
.../sentry/api/endpoints/group_integration_details.py	94.11%	1 Missing and 1 partial ⚠️
src/sentry/integrations/vsts/webhooks.py	81.81%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #80719      +/-   ##
==========================================
+ Coverage   78.39%   78.42%   +0.02%     
==========================================
  Files        7206     7212       +6     
  Lines      319438   319787     +349     
  Branches    43991    44011      +20     
==========================================
+ Hits       250437   250777     +340     
- Misses      62620    62626       +6     
- Partials     6381     6384       +3

iamrajjoshi

i just have a few questions, but i think we are kind of aligned on "halt".

i have been thinking of fault as any unexpected condition that isn't user's fault.
with that definition, i have been considering halt as all other non-success conditions, which makes it probably the broadest in definition.

iamrajjoshi · 2024-11-14T16:27:06Z

src/sentry/api/endpoints/group_integration_details.py

+            try:
+                installation.after_link_issue(external_issue, data=request.data)
+            except IntegrationFormError as exc:
+                lifecycle.record_halt(exc)


are these errors user config errors?

Yeah, typically an IntegrationFormError is an incorrectly configured Jira ticket creation config.

iamrajjoshi · 2024-11-14T16:30:20Z

src/sentry/integrations/jira/utils/api.py

+            item for item in data["changelog"]["items"] if item["field"] == "assignee"
+        )
+        if not assignee_changed:
+            lifecycle.record_halt()


could this possibly be a success? are these normal scenarios where jira doesn't send us assignee?

That's fair. Tbh though, I'm looking at moving this call into the shared util this calls as there are specific lifecycles we probably want to track there, so this is likely to change.

iamrajjoshi · 2024-11-14T16:31:59Z

src/sentry/integrations/jira/utils/api.py

+        assignee = fields.get("assignee")
+
+        if assignee is None:
+            lifecycle.record_halt()


does this mean the ticket was unassigned? why does this have to be a halt?

Yep, this is actually fine and should be recorded as a success.

…nvocations

Also resets changes made to jira utils API file, which is probably the incorrect place to be adding metrics gathering.

…egration details

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

vercel bot deployed to Preview November 14, 2024 00:33 View deployment

github-advanced-security bot found potential problems Nov 14, 2024

View reviewed changes

GabeVillalobos commented Nov 14, 2024

View reviewed changes

iamrajjoshi reviewed Nov 14, 2024

View reviewed changes

GabeVillalobos added 9 commits November 14, 2024 12:47

feat(ecosystem): Adds issue SLOs along with issue provider names

267ccd9

Updates metrics interface to take integration, not installation

65a74ea

Adds new tests plus metrics assertions for ticketing

424ea58

Adds new metrics assertion util file

46a81b1

Removes previously added issue provider name

0bff1c8

Adds sync_status_outbound test

09fdac7

Removes unnecessary util, updates tests to assert on metrics method i…

324b21c

…nvocations

Adds tests for outbound assignee sync task

e000263

Updates record_halts to include messages, fixes broken test

cb387a3

Also resets changes made to jira utils API file, which is probably the incorrect place to be adding metrics gathering.

GabeVillalobos force-pushed the gv/issue-slos branch from 12effc0 to cb387a3 Compare November 14, 2024 20:48

vercel bot deployed to Preview November 14, 2024 20:52 View deployment

Adds tests for sync_assignee_inbound util

09cc6fd

vercel bot deployed to Preview November 14, 2024 23:22 View deployment

Fixes typing issues, adds assertion for metric gathering in group int…

d25c861

…egration details

vercel bot deployed to Preview November 15, 2024 18:49 View deployment

GabeVillalobos added 2 commits November 15, 2024 12:14

Adds metric gathering to inbound status sync flows

d124f60

Removes accidental whitespace

7ec9649

GabeVillalobos marked this pull request as ready for review November 15, 2024 20:20

GabeVillalobos requested review from a team as code owners November 15, 2024 20:20

vercel bot deployed to Preview November 15, 2024 20:21 View deployment

Adds Jira installation metrics

52726ea

vercel bot deployed to Preview November 15, 2024 21:21 View deployment

Adds tests for linking metrics, untested branches

9a64b20

vercel bot deployed to Preview November 15, 2024 22:30 View deployment

Fixes bug with project manager action context

72005db

vercel bot deployed to Preview November 15, 2024 23:16 View deployment

@@ -186,3 +186,4 @@
                             lifecycle.record_failure(e)
-                            return Response({"non_field_errors": [str(e)]}, status=400)
+                            self.log_error(e)
+                            return Response({"non_field_errors": ["An internal error has occurred."]}, status=400)
@@ -249,2 +250,8 @@
+                def log_error(self, error: Exception) -> None:
+                    # Log the detailed error message for debugging purposes
+                    import logging
+                    logger = logging.getLogger(__name__)
+                    logger.error("Integration error occurred: %s", str(error))
                 def post(self, request: Request, group, integration_id) -> Response:

@@ -27,2 +27,3 @@
             from sentry.users.models.user import User
+            import logging
@@ -220,3 +221,4 @@
                             lifecycle.record_failure(e)
-                            return Response({"non_field_errors": [str(e)]}, status=400)
+                            logging.error("IntegrationError: %s", str(e))
+                            return Response({"non_field_errors": ["An internal error has occurred."]}, status=400)
@@ -233,2 +235,3 @@
                             lifecycle.record_halt(exc)
+                            logging.error("IntegrityError: %s", str(exc))
                             return Response({"non_field_errors": ["That issue is already linked"]}, status=400)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ecosystem): Adds Project Management integration SLOs #80719

feat(ecosystem): Adds Project Management integration SLOs #80719

GabeVillalobos commented Nov 14, 2024 •

edited

Loading

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

GabeVillalobos Nov 15, 2024

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

GabeVillalobos Nov 14, 2024

GabeVillalobos Nov 14, 2024

GabeVillalobos Nov 14, 2024

codecov bot commented Nov 14, 2024 •

edited

Loading

iamrajjoshi left a comment

iamrajjoshi Nov 14, 2024

GabeVillalobos Nov 14, 2024

iamrajjoshi Nov 14, 2024

GabeVillalobos Nov 14, 2024

iamrajjoshi Nov 14, 2024

GabeVillalobos Nov 14, 2024

feat(ecosystem): Adds Project Management integration SLOs #80719

Are you sure you want to change the base?

feat(ecosystem): Adds Project Management integration SLOs #80719

Conversation

GabeVillalobos commented Nov 14, 2024 • edited Loading

WIP - Adds Project Management/Issues SLO metrics

Has Metrics, Needs Testing:

Incidental Work:

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

codecov bot commented Nov 14, 2024 • edited Loading

Codecov Report

iamrajjoshi left a 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

GabeVillalobos commented Nov 14, 2024 •

edited

Loading

codecov bot commented Nov 14, 2024 •

edited

Loading