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

feat(migrations): add discover local #5788

Merged
merged 5 commits into from
Apr 17, 2024
Merged

Conversation

dbanda
Copy link
Contributor

@dbanda dbanda commented Apr 17, 2024

Our migrations in SaaS don't have the discover local table. This causes a mismatch between discover migrations in SaaS and self-hosted/CI. The mismatch prevents us from running any new discover migrations.

To fix this , this migration adds a discover_local table if it doesn't exist to reconcile SaaS with the self-hosted migrations. In SaaS the local table will not be used because we merge the distributed tables directly, but this will allow us to unblock discover migrations.

Copy link

github-actions bot commented Apr 17, 2024

This PR has a migration; here is the generated SQL

-- start migrations

-- forward migration discover : 0008_discover_fix_add_local_table
Local op: CREATE TABLE IF NOT EXISTS discover_local (event_id UUID, project_id UInt64, type LowCardinality(String), timestamp DateTime, platform LowCardinality(String), environment LowCardinality(Nullable(String)), release LowCardinality(Nullable(String)), dist LowCardinality(Nullable(String)), transaction_name LowCardinality(String), message String, title String, user String, user_hash UInt64, user_id Nullable(String), user_name Nullable(String), user_email Nullable(String), ip_address_v4 Nullable(IPv4), ip_address_v6 Nullable(IPv6), sdk_name LowCardinality(Nullable(String)), sdk_version LowCardinality(Nullable(String)), http_method LowCardinality(Nullable(String)), http_referer Nullable(String), tags Nested(key String, value String), _tags_hash_map Array(UInt64), contexts Nested(key String, value String), span_id Nullable(UInt64), trace_id Nullable(UUID), deleted UInt8) ENGINE Merge(currentDatabase(), '^errors_local$|^transactions_local$');
-- end forward migration discover : 0008_discover_fix_add_local_table




-- backward migration discover : 0008_discover_fix_add_local_table
-- end backward migration discover : 0008_discover_fix_add_local_table

@dbanda dbanda changed the title migrations: add discover local and new discover table that references it feat(migrations): add discover local and new discover table that references it Apr 17, 2024
@dbanda dbanda changed the title feat(migrations): add discover local and new discover table that references it feat(migrations): add discover local Apr 17, 2024
@dbanda dbanda marked this pull request as ready for review April 17, 2024 15:13
@dbanda dbanda requested a review from a team as a code owner April 17, 2024 15:13
Copy link

codecov bot commented Apr 17, 2024

Test Failures Detected: Due to failing tests, we cannot provide coverage reports at this time.

❌ Failed Test Results:

Completed 1 tests with 1 failed, 0 passed and 0 skipped.

View the full list of failed tests
Test Description Failure message
Testsuite:
pytest
Test name:
tests.test_api.TestApi::test_count
Envs:
- default
No failure message available

@evanh
Copy link
Member

evanh commented Apr 17, 2024

I'm confused about what is going on here, there is already a migration with the local table: https://github.com/getsentry/snuba/blob/master/snuba/snuba_migrations/discover/0001_discover_merge_table.py#L48

@dbanda
Copy link
Contributor Author

dbanda commented Apr 17, 2024

That is correct. We have the migration but we didn't run it in SaaS.

@evanh
Copy link
Member

evanh commented Apr 17, 2024

That is correct. We have the migration but we didn't run it in SaaS.

But then why add the migration again? Why not run the migration in SaaS?

@dbanda
Copy link
Contributor Author

dbanda commented Apr 17, 2024

That is correct. We have the migration but we didn't run it in SaaS.

But then why add the migration again? Why not run the migration in SaaS?

The table has changed since then.

@dbanda dbanda merged commit 41d8479 into master Apr 17, 2024
30 checks passed
@dbanda dbanda deleted the dbanda/saas-add-discover-local branch April 17, 2024 21:19
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.

2 participants