Skip to content

feat(search-issues): Add message column to search issues #4385

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

Merged
merged 2 commits into from
Jun 21, 2023

Conversation

wedamija
Copy link
Member

@wedamija wedamija commented Jun 20, 2023

This adds the message column to search issues, since we missed including it a column initially.

Related to getsentry/sentry#50345

@wedamija wedamija requested a review from a team as a code owner June 20, 2023 23:13
@github-actions
Copy link

This PR has a migration; here is the generated SQL

-- start migrations

-- forward migration search_issues : 0009_add_message
Local op: ALTER TABLE search_issues_local_v2 ADD COLUMN IF NOT EXISTS message String AFTER replay_id;
Distributed op: ALTER TABLE search_issues_dist_v2 ADD COLUMN IF NOT EXISTS message String AFTER replay_id;
-- end forward migration search_issues : 0009_add_message




-- backward migration search_issues : 0009_add_message
Distributed op: ALTER TABLE search_issues_dist_v2 DROP COLUMN IF EXISTS message;
Local op: ALTER TABLE search_issues_local_v2 DROP COLUMN IF EXISTS message;
-- end backward migration search_issues : 0009_add_message

Copy link
Contributor

@barkbarkimashark barkbarkimashark left a comment

Choose a reason for hiding this comment

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

Ahh, damn I missed this column ..

The code looks ok, but I think we'll need to split this PR up into two following steps:

  1. PR1: Add the migration and include it in group_loader.py
  2. Merge PR1 and have someone from the SNS team manually run the migration against prod.
  3. PR2: Start extracting the field in search_issues_processor, and exposing the field in the storage and entity.
  4. Merge PR2 and deploy snuba.

@barkbarkimashark
Copy link
Contributor

For completeness, you probably also wanna add a processor and api test:

def test_extract_replay_id(self, message_base):

def test_eventstream_query_profile_id_replay_id(self) -> None:

wedamija added a commit to getsentry/sentry that referenced this pull request Jun 20, 2023
…nce titles works as expected

This validates that searching for an issue platform issue via text in the title works as expected.
Relies on #51325 and getsentry/snuba#4385

Related to #50345
@wedamija wedamija force-pushed the danf/search-issues-message branch from e251234 to f11cd2d Compare June 20, 2023 23:59
@wedamija
Copy link
Member Author

Ahh, damn I missed this column ..

The code looks ok, but I think we'll need to split this PR up into two following steps:

  1. PR1: Add the migration and include it in group_loader.py
  2. Merge PR1 and have someone from the SNS team manually run the migration against prod.
  3. PR2: Start extracting the field in search_issues_processor, and exposing the field in the storage and entity.
  4. Merge PR2 and deploy snuba.

Tbh I think I missed it in the initial design. Thanks for the feedback, splitting this up now

wedamija added a commit that referenced this pull request Jun 21, 2023
This allows the message column on search issues to be queried, and stops using `issue_title`. We
won't merge this until we've dual written data for a week or two.

Relies on #4385, #4387
Related to getsentry/sentry#50345
@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (3adb381) 90.34% compared to head (e308b76) 90.34%.

❗ Current head e308b76 differs from pull request most recent head ce479ae. Consider uploading reports for the commit ce479ae to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4385   +/-   ##
=======================================
  Coverage   90.34%   90.34%           
=======================================
  Files         802      803    +1     
  Lines       39375    39388   +13     
  Branches      245      245           
=======================================
+ Hits        35573    35585   +12     
- Misses       3770     3771    +1     
  Partials       32       32           
Impacted Files Coverage Δ
snuba/migrations/group_loader.py 95.00% <ø> (ø)
tests/datasets/test_spans_processor.py 96.96% <ø> (ø)
tests/test_consumer.py 100.00% <ø> (ø)
snuba/datasets/processors/spans_processor.py 89.04% <100.00%> (+0.05%) ⬆️
snuba/migrations/clickhouse.py 100.00% <100.00%> (ø)
snuba/snapshots/__init__.py 91.93% <100.00%> (ø)
snuba/snapshots/postgres_snapshot.py 89.87% <100.00%> (ø)
...snuba_migrations/search_issues/0009_add_message.py 100.00% <100.00%> (ø)
tests/consumers/test_consumer_builder.py 98.43% <100.00%> (-1.57%) ⬇️
tests/snapshots/test_postgres_snapshot.py 97.56% <100.00%> (-0.06%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

wedamija added a commit to getsentry/sentry that referenced this pull request Jun 21, 2023
…nce titles works as expected

This validates that searching for an issue platform issue via text in the title works as expected.
Relies on #51325 and getsentry/snuba#4385

Related to #50345
@barkbarkimashark barkbarkimashark self-requested a review June 21, 2023 20:28
@wedamija wedamija merged commit 530118b into master Jun 21, 2023
@wedamija wedamija deleted the danf/search-issues-message branch June 21, 2023 20:43
wedamija added a commit that referenced this pull request Jun 22, 2023
This allows the message column on search issues to be queried, and stops using `issue_title`. We
won't merge this until we've dual written data for a week or two.

Relies on #4385, #4387
Related to getsentry/sentry#50345
wedamija added a commit that referenced this pull request Feb 23, 2024
This allows the message column on search issues to be queried, and stops using `issue_title`. We
won't merge this until we've dual written data for a week or two.

Relies on #4385, #4387
Related to getsentry/sentry#50345
wedamija added a commit that referenced this pull request Feb 26, 2024
)

This allows the message column on search issues to be queried, and stops using `issue_title`. We
won't merge this until we've dual written data for a week or two.

Relies on #4385, #4387
Related to getsentry/sentry#50345
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants