refactor(config): refactor config package #6782

grandwizard28 · 2025-01-08T21:31:55Z

Summary

refactor config package and get rid of upstream dependency on opentelemetry-collector

Important

Refactor configuration package to remove opentelemetry-collector dependency, introduce new SQL store with migrations, and update server initialization.

Configuration Refactor:
- Replaces opentelemetry-collector dependency with koanf and viper for configuration management.
- Introduces pkg/config for configuration handling with support for environment and file providers.
- Adds pkg/signoz for managing application-wide configuration.
Database and Migrations:
- Implements new SQL store in pkg/sqlstore with migration support using bun.
- Adds migration files in pkg/sqlstore/migrations for various components like agents, dashboards, and integrations.
- Refactors database initialization in dao/factory.go and dao/sqlite/connection.go to use new SQL store.
Server and Application Changes:
- Updates server.go to use new configuration and SQL store.
- Refactors logparsingpipeline and opamp components to align with new configuration and database setup.
- Removes old migration logic from migrate/migate.go.

^{This description was created by}^{for 2060905. It will automatically update as commits are pushed.}

github-actions · 2025-01-08T21:32:10Z

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

github-actions · 2025-01-08T21:32:35Z

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

ellipsis-dev

👍 Looks good to me! Reviewed everything up to ae5ba2b in 1 minute and 44 seconds

More details

Looked at 1775 lines of code in 34 files
Skipped 0 files when reviewing.
Skipped posting 4 drafted comments based on config settings.

1. pkg/config/conf.go:28

Draft comment:
The comment on line 28 is misleading. It should clarify that the function merges the input configuration at a specified path within the current configuration.
Reason this comment was not posted:
Confidence changes required: 50%
The comment on line 28 incorrectly states that the function merges the current configuration with the input configuration. This is misleading as the function actually merges the input configuration at a specified path within the current configuration.

2. pkg/config/uri.go:10

Draft comment:
The comment on line 10 is misleading. It should clarify that the regex matches a specific format of URI, not the entire URI definition.
Reason this comment was not posted:
Confidence changes required: 50%
The comment on line 10 in pkg/config/uri.go is misleading. It states that the regex complies with the URI definition, but it only matches a specific format of URI. This should be clarified.

3. pkg/web/config.go:10

Draft comment:
The comment on line 10 is outdated. It should reflect that the Config satisfies the new config.Config interface, not confmap.Config.
Reason this comment was not posted:
Confidence changes required: 50%
The comment on line 10 in pkg/web/config.go is misleading. It states that the Config satisfies the confmap.Config interface, but it should be updated to reflect the new config.Config interface.

4. pkg/web/router/router.go:1

Draft comment:
Avoid using the component/index.tsx file structure approach, as it makes it difficult to debug and find components using global search tools like VS Code.
Reason this comment was not posted:
Comment was on unchanged code.

Workflow ID: wflow_XloMbrlpzoO23qs6

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

pkg/web/router/router.go

+	path := filepath.Join(web.cfg.Directory, req.URL.Path)
+
+	// check whether a file exists or is a directory at the given path
+	fi, err := os.Stat(path)


To fix the problem, we need to ensure that the path constructed from user input is validated to prevent directory traversal attacks. This can be done by resolving the input path with respect to a safe directory and then checking that the resulting path is still within the intended directory.

Define a constant for the safe directory.

Resolve the input path with respect to the safe directory using filepath.Abs.

Check that the resolved path starts with the safe directory path.

If the path is invalid, return an error response.

vikrantgupta25 · 2025-01-09T05:00:15Z

pkg/config/conf.go

+// func (conf *Conf) Unmarshal(input any) error {
+
+// 	return conf.Koanf.UnmarshalWithConf("", input, koanf.UnmarshalConf{
+// 		Tag: "mapstructure",


remove this

github-actions · 2025-01-09T19:32:42Z

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

ellipsis-dev

👍 Looks good to me! Incremental review on 4c94386 in 1 minute and 31 seconds

More details

Looked at 2466 lines of code in 40 files
Skipped 0 files when reviewing.
Skipped posting 4 drafted comments based on config settings.

1. pkg/signoz/signoz.go:24

Draft comment:
Add error handling for unrecognized cache providers to prevent potential nil pointer dereference.
Reason this comment was not posted:
Comment was not on a valid diff hunk.

2. pkg/signoz/signoz.go:46

Draft comment:
Add error handling for web initialization to prevent potential nil pointer dereference.
Reason this comment was not posted:
Comment was not on a valid diff hunk.

3. pkg/signoz/signoz.go:57

Draft comment:
Add error handling for sqlStore initialization to prevent potential nil pointer dereference.
Reason this comment was not posted:
Comment looked like it was already resolved.

4. pkg/query-service/app/server.go:27

Draft comment:
Avoid using the component/index.tsx file structure approach, as it makes it difficult to debug and find components using global search tools like VS Code.
Reason this comment was not posted:
Comment was not on a valid diff hunk.

Workflow ID: wflow_SdvgtI8IbXj32Z5C

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

github-actions · 2025-01-09T20:04:00Z

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

ellipsis-dev

👍 Looks good to me! Incremental review on 5d26d2b in 1 minute and 8 seconds

More details

Looked at 170 lines of code in 5 files
Skipped 0 files when reviewing.
Skipped posting 3 drafted comments based on config settings.

1. pkg/query-service/app/integrations/manager.go:126

Draft comment:
Removing error handling for NewInstalledIntegrationsSqliteRepo can lead to runtime errors if the database is not properly initialized. Consider re-adding error handling to ensure robustness.
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:

Without seeing NewInstalledIntegrationsSqliteRepo implementation, I can't be certain if it can fail. 2. The change suggests the function was modified to not return an error anymore. 3. If the function signature changed, then error handling would be unnecessary. 4. The code compiles, suggesting the function signature must have changed. 5. The PR author likely made this change intentionally after determining errors weren't possible.
I don't have access to the NewInstalledIntegrationsSqliteRepo implementation or its changes. The comment could be valid if that function can still fail at runtime.
Since the code compiles, the function signature must have changed to not return an error. The PR author likely determined error handling wasn't needed and simplified the API.
Without seeing the NewInstalledIntegrationsSqliteRepo implementation or its changes, I cannot verify if error handling is actually needed. The comment makes assumptions without full context.

2. pkg/query-service/app/integrations/test_utils.go:18

Draft comment:
Removing error handling for NewInstalledIntegrationsSqliteRepo can lead to runtime errors if the database is not properly initialized. Consider re-adding error handling to ensure robustness. This is also applicable in NewTestIntegrationsManager.
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
This is a test utility function, and test utilities often simplify error handling for cleaner tests. The database is created using NewQueryServiceDBForTests which likely handles setup validation. Since this is test code, a panic on initialization failure is actually preferable to error propagation. The comment seems to be applying production code standards to test code.
I could be wrong about the database initialization being validated in NewQueryServiceDBForTests. There could be valid reasons to handle errors even in test code.
Even if the database initialization isn't fully validated elsewhere, having a test utility panic on setup failure is a valid and common pattern - it fails fast and makes test failures more obvious.
The comment should be deleted as it's applying overly strict standards to test code where simplified error handling is acceptable and often preferred.

3. pkg/query-service/app/integrations/manager.go:126

Draft comment:
The NewInstalledIntegrationsSqliteRepo function no longer returns an error, so the error handling in NewManager is unnecessary and should be removed.
Reason this comment was not posted:
Comment looked like it was already resolved.

Workflow ID: wflow_BOx5CKlqmlQpyfHW

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

github-actions · 2025-01-09T20:32:52Z

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

ellipsis-dev

👍 Looks good to me! Incremental review on 0c3af05 in 1 minute and 5 seconds

More details

Looked at 106 lines of code in 5 files
Skipped 0 files when reviewing.
Skipped posting 5 drafted comments based on config settings.

1. ee/query-service/app/server.go:210

Draft comment:
The removal of the 'engine' parameter from the NewLogParsingPipelinesController function call is consistent with the refactoring goal. Ensure that this parameter is not used elsewhere in the function.
Reason this comment was not posted:
Confidence changes required: 20%
The removal of the 'engine' parameter from the function signature is consistent across all instances where the function is called. This change aligns with the refactoring goal of removing unnecessary parameters.

2. pkg/query-service/agentConf/manager.go:42

Draft comment:
The removal of the 'DBEngine' field from the ManagerOptions struct is consistent with the refactoring goal. Ensure that this field is not used elsewhere in the code.
Reason this comment was not posted:
Confidence changes required: 20%
The removal of the 'DBEngine' field from the ManagerOptions struct is consistent across all instances where the struct is used. This change aligns with the refactoring goal of removing unnecessary fields.

3. pkg/query-service/app/logparsingpipeline/controller.go:30

Draft comment:
The removal of the 'engine' parameter from the NewLogParsingPipelinesController function call is consistent with the refactoring goal. Ensure that this parameter is not used elsewhere in the function.
Reason this comment was not posted:
Confidence changes required: 20%
The removal of the 'engine' parameter from the function signature is consistent across all instances where the function is called. This change aligns with the refactoring goal of removing unnecessary parameters.

4. pkg/query-service/tests/integration/logparsingpipeline_test.go:487

Draft comment:
The removal of the 'DBEngine' field from the ManagerOptions struct is consistent with the refactoring goal. Ensure that this field is not used elsewhere in the code.
Reason this comment was not posted:
Confidence changes required: 20%
The removal of the 'DBEngine' field from the ManagerOptions struct is consistent across all instances where the struct is used. This change aligns with the refactoring goal of removing unnecessary fields.

5. pkg/query-service/app/server.go:174

Draft comment:
Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. This is also applicable in other parts of the code where inline styles are used.
Reason this comment was not posted:
Comment was not on a valid diff hunk.

Workflow ID: wflow_8M11gtu6Fo1lD3Vp

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

github-actions · 2025-01-09T21:02:27Z

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

ellipsis-dev

👍 Looks good to me! Incremental review on e1cb905 in 38 seconds

More details

Looked at 241 lines of code in 5 files
Skipped 0 files when reviewing.
Skipped posting 3 drafted comments based on config settings.

1. pkg/sqlstore/config.go:36

Draft comment:
The increase in MaxOpenConns from 10 to 100 should be justified. Ensure the system can handle this increase without resource exhaustion.
Reason this comment was not posted:
Confidence changes required: 50%
The change in MaxOpenConns from 10 to 100 in pkg/sqlstore/config.go is significant and should be justified. Increasing the number of open connections can improve performance by allowing more concurrent database operations, but it can also lead to resource exhaustion if not managed properly. It's important to ensure that the system can handle this increase without negative side effects.

2. pkg/sqlstore/provider/sqlite/provider.go:33

Draft comment:
The log message includes ?_foreign_keys=true in the path, which might be misleading as it's not part of the actual connection string. Consider aligning the log message with the actual connection string or explaining the discrepancy.
Reason this comment was not posted:
Confidence changes required: 50%
The connection string for SQLite in pkg/sqlstore/provider/sqlite/provider.go includes ?_foreign_keys=true in the log message but not in the actual connection string. This could lead to confusion as the log message suggests foreign keys are enabled by default, which is not the case unless explicitly set in the connection string or via PRAGMA command.

3. pkg/sqlstore/provider/sqlite/provider.go:5

Draft comment:
Avoid using the component/index.tsx file structure approach, as it makes it difficult to debug and find components using global search tools.
Reason this comment was not posted:
Comment was on unchanged code.

Workflow ID: wflow_WX037G8SS8Azkp3M

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

github-actions · 2025-01-09T21:05:35Z

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

ellipsis-dev

👍 Looks good to me! Incremental review on 2060905 in 36 seconds

More details

Looked at 75 lines of code in 2 files
Skipped 0 files when reviewing.
Skipped posting 2 drafted comments based on config settings.

1. pkg/sqlstore/provider/sqlite/provider.go:30

Draft comment:
The refactor removed the explicit enabling of foreign key support using PRAGMA foreign_keys = ON;. Ensure that foreign key constraints are enforced as expected, as SQLite does not enable them by default.
Reason this comment was not posted:
Comment did not seem useful.

2. pkg/sqlstore/provider/sqlite/provider.go:30

Draft comment:
Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. This comment applies to other instances of inline styles in the code as well.
Reason this comment was not posted:
Comment was on unchanged code.

Workflow ID: wflow_iCItnw1uPH1tsZFP

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

refactor(config): refactor config package

ae5ba2b

ellipsis-dev bot reviewed Jan 8, 2025

View reviewed changes

github-advanced-security bot found potential problems Jan 8, 2025

View reviewed changes

vikrantgupta25 reviewed Jan 9, 2025

View reviewed changes

vikrantgupta25 previously approved these changes Jan 9, 2025

View reviewed changes

refactor(migrations): move migrations into a single package

4c94386

grandwizard28 dismissed vikrantgupta25’s stale review via 4c94386 January 9, 2025 19:31

ellipsis-dev bot reviewed Jan 9, 2025

View reviewed changes

fix(tests): fix unit tests

5d26d2b

ellipsis-dev bot reviewed Jan 9, 2025

View reviewed changes

feat(signoz): remove references to sqlite

0c3af05

ellipsis-dev bot reviewed Jan 9, 2025

View reviewed changes

feat(signoz): make mattn the permanent driver

e1cb905

ellipsis-dev bot reviewed Jan 9, 2025

View reviewed changes

feat(signoz): refactor connection config struct

2060905

ellipsis-dev bot reviewed Jan 9, 2025

View reviewed changes

@@ -7,2 +7,3 @@
             	"path/filepath"
+            	"strings"
             	"time"
@@ -73,7 +74,11 @@
             func (web *router) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
-            	// Join internally call path.Clean to prevent directory traversal
-            	path := filepath.Join(web.cfg.Directory, req.URL.Path)
+            	// Resolve the user-provided path with respect to the safe directory
+            	absPath, err := filepath.Abs(filepath.Join(web.cfg.Directory, req.URL.Path))
+            	if err != nil || !strings.HasPrefix(absPath, web.cfg.Directory) {
+            		http.Error(rw, "Invalid file path", http.StatusBadRequest)
+            		return
+            	}
             	// check whether a file exists or is a directory at the given path
-            	fi, err := os.Stat(path)
+            	fi, err := os.Stat(absPath)
             	if os.IsNotExist(err) || fi.IsDir() {

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(config): refactor config package #6782

refactor(config): refactor config package #6782

grandwizard28 commented Jan 8, 2025 •

edited by ellipsis-dev bot

Loading

github-actions bot commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

ellipsis-dev bot left a comment

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

vikrantgupta25 Jan 9, 2025

github-actions bot commented Jan 9, 2025

ellipsis-dev bot left a comment

github-actions bot commented Jan 9, 2025

ellipsis-dev bot left a comment

github-actions bot commented Jan 9, 2025

ellipsis-dev bot left a comment

github-actions bot commented Jan 9, 2025

ellipsis-dev bot left a comment

github-actions bot commented Jan 9, 2025

ellipsis-dev bot left a comment

refactor(config): refactor config package #6782

Are you sure you want to change the base?

refactor(config): refactor config package #6782

Conversation

grandwizard28 commented Jan 8, 2025 • edited by ellipsis-dev bot Loading

Summary

github-actions bot commented Jan 8, 2025

github-actions bot commented Jan 8, 2025

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

vikrantgupta25 Jan 9, 2025

Choose a reason for hiding this comment

github-actions bot commented Jan 9, 2025

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 9, 2025

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 9, 2025

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 9, 2025

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 9, 2025

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

grandwizard28 commented Jan 8, 2025 •

edited by ellipsis-dev bot

Loading