feat(.): initialize all factories #6844
Conversation
feat(.): initialize all factories feat(.): initialize all factories feat(.): initialize all factories
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to c79b3c9 in 1 minute and 52 seconds
More details
- Looked at
2383lines of code in45files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. ee/query-service/app/server.go:82
- Draft comment:
TheSkipWebFrontendoption has been removed fromServerOptions. Ensure that this change is intentional and that there is no longer a need for this option, as it might affect users who relied on it to skip the web frontend. - Reason this comment was not posted:
Comment did not seem useful.
2. ee/query-service/main.go:115
- Draft comment:
TheSkipWebFrontendflag has been removed. Ensure that this change is intentional and that there is no longer a need for this option, as it might affect users who relied on it to skip the web frontend. - Reason this comment was not posted:
Marked as duplicate.
3. pkg/web/routerweb/provider.go:1
- Draft comment:
Avoid using thecomponent/index.tsxfile 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_eWEfGSE0dvgzXcVL
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 3e80b23 in 40 seconds
More details
- Looked at
401lines of code in10files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. pkg/registry/registry.go:8
- Draft comment:
Theerrsslice is initialized with a length equal to the number of services, but it is then appended to, which will result in a slice with a length greater than expected. Initializeerrswith a length of 0 instead:
errs := make([]error, 0, len(r.services))
- Reason this comment was not posted:
Comment was not on a valid diff hunk.
2. pkg/http/server/config.go:1
- Draft comment:
Avoid using thecomponent/index.tsxfile structure approach, as it makes it difficult to debug and find components using global search tools. This is applicable in other files as well. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_Vv4JErJ7looX30Pe
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
the changes are big, please break it down. I see changes like files colde chages from old packages. Please create pr's to update then indivuidally. |
|
Apart from moving sqlmigrator and sqlmigration to the root level, other changes have to be done in one go since we changing the config interface. Again we are initializing nothing new, so the only impact is on Web and Cache. |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 7316765 in 50 seconds
More details
- Looked at
113lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. conf/example.yaml:58
- Draft comment:
Storing sensitive information like passwords in configuration files is a security risk. Consider using environment variables or a secrets management tool. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
Since this is an example configuration file (clearly marked "Do not modify this file"), it's not appropriate to warn about security practices here. The password field is actually empty, showing it as an optional field. This is exactly what you'd expect in an example config. The comment is making a general security recommendation that isn't relevant for an example file.
The comment does raise a valid general security concern. Maybe the example file should explicitly mention using environment variables as a best practice?
While security is important, example files are meant to show structure and available options, not enforce best practices. The empty password field actually implies it's optional.
Delete the comment as it's raising security concerns about an example configuration file that's not meant to be used in production.
2. conf/example.yaml:1
- Draft comment:
The configuration file is well-structured and does not violate any of the specified rules. - Reason this comment was not posted:
Confidence changes required:0%
The file structure and content do not violate any of the specified rules. The configuration file is well-structured and does not involve any inline styles, hardcoded colors, or inappropriate interface modifications.
Workflow ID: wflow_Z8W1lRvWMmQIBJlE
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 9dccd95 in 26 seconds
More details
- Looked at
263lines of code in2files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_goJ5bOMQuef71YON
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 64035e7 in 30 seconds
More details
- Looked at
37lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. pkg/factory/provider.go:28
- Draft comment:
Consider making logging optional based on a configuration setting to avoid unnecessary logging in production environments. - Reason this comment was not posted:
Confidence changes required:50%
The logging statements in theNewmethod are useful for debugging, but they should be optional to avoid unnecessary logging in production environments. Consider adding a condition to enable or disable logging based on a configuration setting.
2. pkg/factory/provider.go:28
- Draft comment:
The code is well-structured and follows the specified rules. No changes are necessary. - Reason this comment was not posted:
Confidence changes required:0%
The code does not violate any of the specified rules. It uses structured logging with zap, which is appropriate for the context.
Workflow ID: wflow_nbQRJRyGKX6GhZiL
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 910c9ef in 45 seconds
More details
- Looked at
15lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. pkg/config/envprovider/provider_test.go:36
- Draft comment:
The test sets environment variablesK1_K2andK3_K4but expects an empty map. This seems incorrect as the environment variables should be reflected in the result. Please verify the expected behavior. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_ZBUVECgwm3ndn5XJ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| SQLMigrationProviderFactories factory.NamedMap[factory.ProviderFactory[sqlmigration.SQLMigration, sqlmigration.Config]] | ||
| } | ||
|
|
||
| func NewProviderConfig() ProviderConfig { |
There was a problem hiding this comment.
@nityanandagohain I moved this from main.go to here, so that we can reuse this same function in OSS and EE query service.
| @@ -0,0 +1,70 @@ | |||
| ##################### SigNoz Configuration Example ##################### | |||
There was a problem hiding this comment.
@nityanandagohain this has been changed to example since the defaults are persisted alongside the code and not here.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 08fc349 in 23 seconds
More details
- Looked at
16lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. pkg/signoz/provider_test.go:13
- Draft comment:
Consider adding assertions to verify the state of the configuration afterNewProviderConfig()is called, to ensure it initializes as expected. - Reason this comment was not posted:
Confidence changes required:50%
The test functionTestNewProviderConfigis checking ifNewProviderConfigcan be called without causing a panic. However, it doesn't verify if the configuration is correct or if the factories are initialized as expected. Adding assertions to check the state of the configuration after initialization would make the test more robust.
2. pkg/signoz/provider_test.go:9
- Draft comment:
The code adheres to the specified rules. No changes required. - Reason this comment was not posted:
Confidence changes required:0%
The code provided does not violate any of the specified rules. It is a test function and does not involve any UI components or ClickHouseReader interface modifications.
Workflow ID: wflow_PgAw7uq17eIz5KWJ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Summary
feat(.): initialize all factories
Related Issues / PR's
Removed all redundant commits of #6843
Closes #6782
Screenshots
NA
Affected Areas and Manually Tested Areas
Important
Refactor SigNoz configuration and initialization using a new modular system with providers and factories, enhancing flexibility and maintainability.
koanfandmapstructureinpkg/config.envproviderandfileproviderfor configuration loading from environment and files.ResolverandProviderpattern for flexible configuration management.pkg/cache.memorycacheandrediscacheproviders with tests.pkg/web.routerwebandnoopwebproviders with tests.pkg/sqlmigration.pkg/query-service/app/server.goto use new configuration system.go.modto include new dependencies.This description was created by
for 08fc349. It will automatically update as commits are pushed.