Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🔧 chore: remove redundant slack logging & metrics #82744

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

iamrajjoshi
Copy link
Member

we have completed the audit of our integration slos, so going back and cleaning up redundant logging and metrics.

we should also monitor our DD monitors to see if they need updating.

@iamrajjoshi iamrajjoshi self-assigned this Dec 31, 2024
@iamrajjoshi iamrajjoshi requested review from a team as code owners December 31, 2024 00:12
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Dec 31, 2024
Copy link

codecov bot commented Dec 31, 2024

❌ 3 Tests Failed:

Tests completed Failed Passed Skipped
23468 3 23465 214
View the top 3 failed tests by shortest run time
tests.sentry.integrations.slack.service.test_slack_service.TestHandleParentNotification::test_handles_parent_notification_sdk
Stack Traces | 2.73s run time
#x1B[1m#x1B[.../slack/service/test_slack_service.py#x1B[0m:255: in test_handles_parent_notification_sdk
    assert_slo_metric(mock_record, EventLifecycleOutcome.SUCCESS)
#x1B[1m#x1B[.../sentry/testutils/asserts.py#x1B[0m:93: in assert_slo_metric
    assert len(mock_record.mock_calls) == 2
#x1B[1m#x1B[31mE   AssertionError: assert 0 == 2#x1B[0m
#x1B[1m#x1B[31mE    +  where 0 = len([])#x1B[0m
#x1B[1m#x1B[31mE    +    where [] = <MagicMock name='record_event' id='140232433138096'>.mock_calls#x1B[0m
tests.sentry.integrations.slack.service.test_slack_service.TestHandleParentNotification::test_handles_parent_notification_sdk_error
Stack Traces | 2.83s run time
#x1B[1m#x1B[.../slack/service/test_slack_service.py#x1B[0m:269: in test_handles_parent_notification_sdk_error
    assert_slo_metric(mock_record, EventLifecycleOutcome.FAILURE)
#x1B[1m#x1B[.../sentry/testutils/asserts.py#x1B[0m:93: in assert_slo_metric
    assert len(mock_record.mock_calls) == 2
#x1B[1m#x1B[31mE   AssertionError: assert 0 == 2#x1B[0m
#x1B[1m#x1B[31mE    +  where 0 = len([])#x1B[0m
#x1B[1m#x1B[31mE    +    where [] = <MagicMock name='record_event' id='140232433023648'>.mock_calls#x1B[0m
tests.sentry.integrations.slack.service.test_slack_service.TestNotifyAllThreadsForActivity::test_calls_handle_parent_notification_sdk_client
Stack Traces | 2.83s run time
#x1B[1m#x1B[.../slack/service/test_slack_service.py#x1B[0m:176: in test_calls_handle_parent_notification_sdk_client
    assert_slo_metric(mock_record, EventLifecycleOutcome.SUCCESS)
#x1B[1m#x1B[.../sentry/testutils/asserts.py#x1B[0m:93: in assert_slo_metric
    assert len(mock_record.mock_calls) == 2
#x1B[1m#x1B[31mE   AssertionError: assert 4 == 2#x1B[0m
#x1B[1m#x1B[31mE    +  where 4 = len([call(<EventLifecycleOutcome.STARTED: 'STARTED'>),\n call(<EventLifecycleOutcome.SUCCESS: 'SUCCESS'>, None),\n call(<EventLifecycleOutcome.STARTED: 'STARTED'>),\n call(<EventLifecycleOutcome.SUCCESS: 'SUCCESS'>, None)])#x1B[0m
#x1B[1m#x1B[31mE    +    where [call(<EventLifecycleOutcome.STARTED: 'STARTED'>),\n call(<EventLifecycleOutcome.SUCCESS: 'SUCCESS'>, None),\n call(<EventLifecycleOutcome.STARTED: 'STARTED'>),\n call(<EventLifecycleOutcome.SUCCESS: 'SUCCESS'>, None)] = <MagicMock name='record_event' id='140307752261760'>.mock_calls#x1B[0m

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

thread_ts=parent_notification.message_identifier,
text=notification_to_send,
blocks=json_blocks,
)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

diff is weird here, but basically we don't catch the SlackAPI in this function, rather the callee function above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants