From 3e7f3fdf77f0ec4fcc33bd15df1ffaa5e3733fae Mon Sep 17 00:00:00 2001 From: Armen Zambrano G Date: Fri, 24 Feb 2023 14:19:04 -0500 Subject: [PATCH 1/6] WIP --- src/sentry/integrations/github/webhook.py | 16 ++++++++++++++ src/sentry/utils/sdk.py | 26 ++++++++++++++++++++++- 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/src/sentry/integrations/github/webhook.py b/src/sentry/integrations/github/webhook.py index 7bee3f156b988e..e955794f257990 100644 --- a/src/sentry/integrations/github/webhook.py +++ b/src/sentry/integrations/github/webhook.py @@ -14,6 +14,7 @@ from django.views.decorators.csrf import csrf_exempt from django.views.generic import View from rest_framework.request import Request +from sentry_sdk import configure_scope from sentry import options from sentry.constants import ObjectStatus @@ -30,12 +31,26 @@ from sentry.shared_integrations.exceptions import ApiError from sentry.utils import json from sentry.utils.json import JSONData +from sentry.utils.sdk import report_tags_already_set from .repository import GitHubRepositoryProvider logger = logging.getLogger("sentry.webhooks") +def clear_tags_and_context_if_already_set() -> None: + with configure_scope() as scope: + report_tags_already_set(scope) + for tag in ["organization", "organization.slug"]: + if tag in scope._tags: + del scope._tags[tag] + + if "organization" in scope._contexts: + del scope._contexts["organization"] + + logger.info("We've reset the context and tags.") + + class Webhook: provider = "github" @@ -456,6 +471,7 @@ def get_secret(self) -> str | None: raise NotImplementedError def handle(self, request: Request) -> HttpResponse: + clear_tags_and_context_if_already_set() secret = self.get_secret() if secret is None: diff --git a/src/sentry/utils/sdk.py b/src/sentry/utils/sdk.py index 8bd776fe7039d3..f6d7e86d425dc1 100644 --- a/src/sentry/utils/sdk.py +++ b/src/sentry/utils/sdk.py @@ -1,5 +1,6 @@ import copy import inspect +import logging import random import sentry_sdk @@ -8,7 +9,13 @@ # Reexport sentry_sdk just in case we ever have to write another shim like we # did for raven -from sentry_sdk import capture_exception, capture_message, configure_scope, push_scope # NOQA +from sentry_sdk import ( # NOQA + Scope, + capture_exception, + capture_message, + configure_scope, + push_scope, +) from sentry_sdk.client import get_options from sentry_sdk.transport import make_transport from sentry_sdk.utils import logger as sdk_logger @@ -18,6 +25,8 @@ from sentry.utils.db import DjangoAtomicIntegration from sentry.utils.rust import RustInfoIntegration +logger = logging.getLogger(__name__) + UNSAFE_FILES = ( "sentry/event_manager.py", "sentry/tasks/process_buffer.py", @@ -445,11 +454,26 @@ def _kwargs_into_scope(self, scope, extra=None, tags=None, fingerprint=None, req scope.fingerprint = fingerprint +def check_tag(tag_key: str, expected_value: str): + if scope._tags and tag_key in scope._tags and scope._tags["organization"] != expected_value: + extra = {tag_key: scope._tags[tag_key], f"new_{tag_key}": expected_value} + logger.warning(f"Tag already set and different ({tag_key}).", extra=extra) + + +# We have some events being tagged with organization.slug even when we don't have such info +def log_if_tags_already_set(scope: Scope, org_id: int, org_slug: str) -> None: + """If the tags are already set and differ, report them as a Sentry error.""" + check_tag("organization", org_id) + check_tag("organization.slug", org_slug) + + def bind_organization_context(organization): helper = settings.SENTRY_ORGANIZATION_CONTEXT_HELPER # XXX(dcramer): this is duplicated in organizationContext.jsx on the frontend with sentry_sdk.configure_scope() as scope: + log_if_tags_already_set(scope, organization.id, organization.slug) + scope.set_tag("organization", organization.id) scope.set_tag("organization.slug", organization.slug) scope.set_context("organization", {"id": organization.id, "slug": organization.slug}) From 444417a8d823824c1ab658b87e1daa18cb2b2a9a Mon Sep 17 00:00:00 2001 From: Armen Zambrano G Date: Mon, 27 Feb 2023 12:50:51 -0500 Subject: [PATCH 2/6] Reduce changes --- src/sentry/integrations/github/webhook.py | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/src/sentry/integrations/github/webhook.py b/src/sentry/integrations/github/webhook.py index e955794f257990..7bee3f156b988e 100644 --- a/src/sentry/integrations/github/webhook.py +++ b/src/sentry/integrations/github/webhook.py @@ -14,7 +14,6 @@ from django.views.decorators.csrf import csrf_exempt from django.views.generic import View from rest_framework.request import Request -from sentry_sdk import configure_scope from sentry import options from sentry.constants import ObjectStatus @@ -31,26 +30,12 @@ from sentry.shared_integrations.exceptions import ApiError from sentry.utils import json from sentry.utils.json import JSONData -from sentry.utils.sdk import report_tags_already_set from .repository import GitHubRepositoryProvider logger = logging.getLogger("sentry.webhooks") -def clear_tags_and_context_if_already_set() -> None: - with configure_scope() as scope: - report_tags_already_set(scope) - for tag in ["organization", "organization.slug"]: - if tag in scope._tags: - del scope._tags[tag] - - if "organization" in scope._contexts: - del scope._contexts["organization"] - - logger.info("We've reset the context and tags.") - - class Webhook: provider = "github" @@ -471,7 +456,6 @@ def get_secret(self) -> str | None: raise NotImplementedError def handle(self, request: Request) -> HttpResponse: - clear_tags_and_context_if_already_set() secret = self.get_secret() if secret is None: From 4c651577ebe3dd1cf15d15d2c8010a8ae5abc56c Mon Sep 17 00:00:00 2001 From: Armen Zambrano G Date: Mon, 27 Feb 2023 13:00:46 -0500 Subject: [PATCH 3/6] Report error --- src/sentry/utils/sdk.py | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/src/sentry/utils/sdk.py b/src/sentry/utils/sdk.py index f6d7e86d425dc1..11be9f9b8b253d 100644 --- a/src/sentry/utils/sdk.py +++ b/src/sentry/utils/sdk.py @@ -454,17 +454,32 @@ def _kwargs_into_scope(self, scope, extra=None, tags=None, fingerprint=None, req scope.fingerprint = fingerprint -def check_tag(tag_key: str, expected_value: str): - if scope._tags and tag_key in scope._tags and scope._tags["organization"] != expected_value: - extra = {tag_key: scope._tags[tag_key], f"new_{tag_key}": expected_value} - logger.warning(f"Tag already set and different ({tag_key}).", extra=extra) +def check_tag(tag_key: str, expected_value: str) -> bool: + """Detect a tag already set and being different than what we expect. + + This function checks if a tag has been already been set and if it differs + from what we want to set it to. + """ + with configure_scope() as scope: + if scope._tags and tag_key in scope._tags and scope._tags[tag_key] != expected_value: + extra = { + f"previous_{tag_key}": scope._tags[tag_key], + f"new_{tag_key}": expected_value, + } + logger.warning(f"Tag already set and different ({tag_key}).", extra=extra) + return True # We have some events being tagged with organization.slug even when we don't have such info def log_if_tags_already_set(scope: Scope, org_id: int, org_slug: str) -> None: """If the tags are already set and differ, report them as a Sentry error.""" - check_tag("organization", org_id) - check_tag("organization.slug", org_slug) + try: + if check_tag("organization", org_id) or check_tag("organization.slug", org_slug): + raise Exception( + "This is an intended error for investigating what is the root of this problem." + ) + except Exception as e: + logger.exception(e) def bind_organization_context(organization): From f215b696b8fc10b936604cc833afe4c89e8e2ffd Mon Sep 17 00:00:00 2001 From: Armen Zambrano G Date: Mon, 27 Feb 2023 15:21:39 -0500 Subject: [PATCH 4/6] Set tag and use error instead of exception --- src/sentry/utils/sdk.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/sentry/utils/sdk.py b/src/sentry/utils/sdk.py index 11be9f9b8b253d..8beb8143492c03 100644 --- a/src/sentry/utils/sdk.py +++ b/src/sentry/utils/sdk.py @@ -467,19 +467,16 @@ def check_tag(tag_key: str, expected_value: str) -> bool: f"new_{tag_key}": expected_value, } logger.warning(f"Tag already set and different ({tag_key}).", extra=extra) + # This can be used to find errors that may have been mistagged + scope.set_tag("possible_mistag", True) return True # We have some events being tagged with organization.slug even when we don't have such info def log_if_tags_already_set(scope: Scope, org_id: int, org_slug: str) -> None: """If the tags are already set and differ, report them as a Sentry error.""" - try: - if check_tag("organization", org_id) or check_tag("organization.slug", org_slug): - raise Exception( - "This is an intended error for investigating what is the root of this problem." - ) - except Exception as e: - logger.exception(e) + if check_tag("organization", org_id) or check_tag("organization.slug", org_slug): + logger.error("Intentional error for investigation. Ignore it. WOR-2464.") def bind_organization_context(organization): From 2baf267060753c3662036cfea1c17b8d381028a1 Mon Sep 17 00:00:00 2001 From: Armen Zambrano G Date: Mon, 27 Feb 2023 15:23:09 -0500 Subject: [PATCH 5/6] Do not import Scope --- src/sentry/utils/sdk.py | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/sentry/utils/sdk.py b/src/sentry/utils/sdk.py index 8beb8143492c03..7577ceee43a7a4 100644 --- a/src/sentry/utils/sdk.py +++ b/src/sentry/utils/sdk.py @@ -9,13 +9,7 @@ # Reexport sentry_sdk just in case we ever have to write another shim like we # did for raven -from sentry_sdk import ( # NOQA - Scope, - capture_exception, - capture_message, - configure_scope, - push_scope, -) +from sentry_sdk import capture_exception, capture_message, configure_scope, push_scope # NOQA from sentry_sdk.client import get_options from sentry_sdk.transport import make_transport from sentry_sdk.utils import logger as sdk_logger @@ -473,10 +467,10 @@ def check_tag(tag_key: str, expected_value: str) -> bool: # We have some events being tagged with organization.slug even when we don't have such info -def log_if_tags_already_set(scope: Scope, org_id: int, org_slug: str) -> None: +def log_if_tags_already_set(org_id: int, org_slug: str) -> None: """If the tags are already set and differ, report them as a Sentry error.""" if check_tag("organization", org_id) or check_tag("organization.slug", org_slug): - logger.error("Intentional error for investigation. Ignore it. WOR-2464.") + logger.error("Intentional error for investigation. See WOR-2464.") def bind_organization_context(organization): From 3654afe8fed6ad35e4ebe29e7732d24593d05b4a Mon Sep 17 00:00:00 2001 From: Armen Zambrano G Date: Tue, 28 Feb 2023 08:48:35 -0500 Subject: [PATCH 6/6] Too many arguments --- src/sentry/utils/sdk.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/utils/sdk.py b/src/sentry/utils/sdk.py index 7577ceee43a7a4..3c83d4b03c1e9d 100644 --- a/src/sentry/utils/sdk.py +++ b/src/sentry/utils/sdk.py @@ -478,7 +478,7 @@ def bind_organization_context(organization): # XXX(dcramer): this is duplicated in organizationContext.jsx on the frontend with sentry_sdk.configure_scope() as scope: - log_if_tags_already_set(scope, organization.id, organization.slug) + log_if_tags_already_set(organization.id, organization.slug) scope.set_tag("organization", organization.id) scope.set_tag("organization.slug", organization.slug)