-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat(sqlmigration): consolidate all sqlmigrations into one package #7018
Conversation
025268e
to
d05ba47
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on d44e1e8 in 1 minute and 9 seconds
More details
- Looked at
253
lines of code in7
files - Skipped
0
files when reviewing. - Skipped posting
11
drafted comments based on config settings.
1. pkg/signoz/config.go:104
- Draft comment:
Consider using a structured logger (e.g., zap.L()) instead of fmt.Println for deprecation warnings for consistency and better production logging. - Reason this comment was not posted:
Comment was on unchanged code.
2. pkg/telemetrystore/config.go:60
- Draft comment:
Ensure that the hardcoded provider value ('clickhouse') in Validate() is intentional; otherwise, consider supporting additional providers or clarifying the limitation in documentation. - Reason this comment was not posted:
Confidence changes required:33%
None
3. pkg/telemetrystore/telemetrystore.go:10
- Draft comment:
The TelemetryStore interface focuses solely on ClickHouse; ensure that this minimal interface aligns with project design and that no non–ClickHouse functions are inadvertently added. - Reason this comment was not posted:
Confidence changes required:33%
None
4. pkg/telemetrystore/telemetrystoretest/provider.go:29
- Draft comment:
The mock provider implementation looks correct; just ensure type assertions on 'p.mock' are safe under all testing scenarios. - Reason this comment was not posted:
Confidence changes required:0%
None
5. pkg/telemetrystore/telemetrystoretest/provider_test.go:29
- Draft comment:
Tests for provider instantiation and methods are adequate; ensure edge cases are covered in future tests if mocking becomes more complex. - Reason this comment was not posted:
Confidence changes required:0%
None
6. ee/query-service/app/server.go:150
- Draft comment:
Removal of the STORAGE env variable check simplifies initialization and enforces use of ClickHouse. Ensure that all related deployment documentation and deprecation notices are updated accordingly. - Reason this comment was not posted:
Comment did not seem useful.
7. pkg/query-service/app/server.go:125
- Draft comment:
The conditional check based on the STORAGE env variable was removed and the clickhouseReader is now used unconditionally. Verify that this change is reflected in your documentation and that users are aware that only ClickHouse is supported. - Reason this comment was not posted:
Marked as duplicate.
8. pkg/query-service/app/server.go:588
- Draft comment:
Replace the use of fmt.Println with structured logging (e.g., using zap.L().Info) for consistent log output. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. pkg/signoz/config.go:103
- Draft comment:
Consider replacing fmt.Println calls in mergeAndEnsureBackwardCompatibility with structured logger calls (e.g., zap.L().Warn) to maintain consistency with the rest of the codebase. - Reason this comment was not posted:
Marked as duplicate.
10. pkg/telemetrystore/config.go:46
- Draft comment:
The Validate() method restricts Provider to 'clickhouse'. Consider using a constant for 'clickhouse' rather than a magic string to ease future changes and improve maintainability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. pkg/telemetrystore/telemetrystoretest/provider.go:30
- Draft comment:
The type assertion in ClickHouseDB() directly casts p.mock to clickhouse.Conn. Consider performing a safe type assertion to guard against potential panics if the underlying type changes. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_PsR1S8kuGsXWNF0V
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on 42fe92c in 23 seconds
More details
- Looked at
13
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. pkg/telemetrystore/config.go:54
- Draft comment:
Ensure that switching the DSN scheme to 'tcp://' aligns with ClickHouse requirements. Confirm that no downstream dependencies expect the 'http://' format. - Reason this comment was not posted:
Comment did not seem useful.
2. pkg/telemetrystore/config.go:54
- Draft comment:
Changed DSN from 'http' to 'tcp' for accurate ClickHouse connection. Consider adding a comment on this choice. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_azqFivtbcH24mm8i
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Few minor comments.
I would suggest deploying this to a fresh instance and testing before merging. Just to avoid any misses.
Summary
factory.Config
interfaceRelated Issues / PR's
Important
Consolidate SQL migrations into a single package, update configuration validation, and streamline test setup for SQLite databases.
server.go
,modelDao.go
, anddb.go
.factory.Config
interface inconfig.go
.testutils.go
to use new migration setup.config.go
to support onlyclickhouse
provider.config.go
.provider.go
to include new SQL migration factories.This description was created by for 42fe92c. It will automatically update as commits are pushed.