feat(preference): add multi-tenant preference module

[vikrantgupta25]

Summary

• make the preference package multi tenant

[vikrantgupta25]

contributes to - https://github.com/SigNoz/platform-pod/issues/460

[ellipsis-dev]

❌ Changes requested. Reviewed everything up to 1433a5b in 3 minutes and 31 seconds

More details

• Looked at 855 lines of code in 12 files
• Skipped 0 files when reviewing.
• Skipped posting 16 drafted comments based on config settings.

1. pkg/query-service/app/preferences/model.go:180

• Draft comment:
  Ensure that string conversions in SanitizeValue handle edge cases (e.g. unexpected falsy boolean values) consistently. The additional check for "true" improves behavior, but consider documenting the supported input types.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
  The suggestion to add integer 1 as a truthy value is reasonable since "1" is already accepted. Documentation would be nice but isn't strictly necessary since the code is fairly self-explanatory. However, this is more of an enhancement suggestion rather than fixing a clear issue.
  The comment doesn't identify any actual bugs or serious issues. The current implementation, while potentially incomplete, works correctly for the common cases.
  While the suggestions would make the code more robust, they aren't critical changes and the current implementation is functional.
  The comment should be deleted as it suggests nice-to-have improvements rather than identifying actual problems that need to be fixed.

2. pkg/query-service/app/preferences/model.go:280

• Draft comment:
  Using json.Marshal for storing preference values is acceptable; however, consider error logging or additional context in error messages to ease debugging when encoding fails.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

3. pkg/sqlmigration/021_update_preferences.go:85

• Draft comment:
  In AddPrimaryKey, use of a callback to perform additional operations is clever. Ensure that the unique index creation (line 109) handles table naming conventions robustly in all environments.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the author to ensure that the unique index creation handles table naming conventions robustly. This is a general request for confirmation and does not provide a specific suggestion or point out a specific issue. It violates the rule against asking the author to ensure behavior is intended.

4. pkg/sqlmigration/021_update_preferences.go:125

• Draft comment:
  Ensure similar validation for user preferences migration—consistent error handling and index creation across both org and user migrations.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

5. pkg/sqlstore/postgressqlstore/dialect.go:231

• Draft comment:
  In UpdatePrimaryKey, check if column type equals Text to decide not updating. Confirm that this is the intended safeguard for when the key is already of type text.
• 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.

6. pkg/sqlstore/sqlitesqlstore/dialect.go:113

• Draft comment:
  The use of PRAGMA table_info to get column type works well; verify that this behavior remains consistent across all SQLite versions in use.
• 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.

7. pkg/sqlstore/sqlstore.go:17

• Draft comment:
  The SQLStore interface cleanly separates concerns by exposing multiple DB interfaces. Ensure that legacy code using SQLxDB is gradually migrated to BunDB where possible.
• Reason this comment was not posted:
  Confidence changes required: 40% <= threshold 50%
  None

8. pkg/types/preference.go:6

• Draft comment:
  Both UserPreference and OrgPreference embed 'Identifiable'. Ensure that the migration from composite primary key to a surrogate key (via Identifiable) is well-documented and backward-compatible.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the author to ensure that the migration is well-documented and backward-compatible. This falls under the rule of not asking the author to ensure things are tested or documented. Therefore, this comment should be removed.

9. pkg/types/preference.go:6

• Draft comment:
  The new use of the Identifiable embed replaces the composite primary key. Ensure this change is intentional and that all queries and migrations are updated accordingly.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 30% <= threshold 50%
  This comment is asking the author to ensure that the change is intentional and that all queries and migrations are updated. This violates the rule against asking the author to ensure the behavior is intended or to double-check things. However, it does point out a specific change regarding the use of Identifiable embed replacing a composite primary key, which could be a significant change. The comment could be rephrased to focus on confirming the intention of replacing the composite primary key without asking the author to ensure updates.

10. pkg/types/preference.go:15

• Draft comment:
  Verify that OrgPreference with Identifiable and a notnull OrgID meets your multi-tenant requirements and that downstream logic handles the new primary key semantics.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to verify their multi-tenant requirements and downstream logic, which violates the rule against asking for confirmation or verification of intentions. It does not provide a specific code suggestion or point out a specific issue that needs addressing.

11. pkg/query-service/app/preferences/model.go:341

• Draft comment:
  Typo: The comment uses 'wherver' instead of 'wherever'. Please correct the spelling in the comments (e.g., in GetAllOrgPreferences and GetAllUserPreferences).
• Reason this comment was not posted:
  Comment was on unchanged code.

12. pkg/query-service/app/preferences/model.go:500

• Draft comment:
  Duplicate Comment: There are two consecutive comments stating "fetch all the org preference values stored in org_preference table" in the GetAllUserPreferences function. Please remove the duplicate to improve clarity.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While the comment is technically correct about the duplication, this is a very minor stylistic issue. The duplicate comments don't affect functionality or maintainability in any significant way. The rules say to only keep comments that clearly require code changes, and to not make purely informative comments.
  The duplicate comments could be seen as a minor code quality issue that makes the code slightly less clean and professional. Removing one would improve readability slightly.
  However, this is an extremely minor stylistic issue that doesn't impact functionality or maintainability in any meaningful way. The rules specifically say not to make purely informative comments.
  This comment should be deleted as it points out a very minor stylistic issue rather than a substantive problem requiring code changes.

13. pkg/signoz/provider.go:57

• Draft comment:
  Typo suggestion: The factory function name 'NewAddPatsFactory()' might be a misspelling. If this is meant to be something like 'NewAddPathsFactory()', please double-check and correct the naming if needed.
• 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.

14. pkg/sqlmigration/sqlmigration.go:16

• Draft comment:
  Minor typographical error: The comment on line 16 lacks a space after the '//' marker (i.e., it reads //in a dedicated ... instead of // in a dedicated ...). This makes the comment slightly less readable.
• 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.

15. pkg/sqlstore/postgressqlstore/dialect.go:42

• Draft comment:
  The comment on line 42 says "if the columns is integer then do this". Consider changing it to "if the column is integer then do this" for grammatical correctness.
• 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.

16. pkg/sqlstore/sqlitesqlstore/dialect.go:41

• Draft comment:
  Minor: The comment at line 41 currently reads "if the columns is integer then do this". Consider revising it to "if the column is integer then do this" for clarity.
• 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.

Workflow ID: wflow_nD3b7J2mWef06YWK

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 7e1e106 in 44 seconds

More details

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

1. pkg/sqlmigration/021_update_preferences.go:48

• Draft comment:
  Enforces non-null constraint on UserID in newUserPreference. Ensure existing data complies with this rule.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to ensure that existing data complies with a new constraint, which is against the rules. It doesn't provide a specific code suggestion or point out a specific issue with the code.

2. pkg/types/preference.go:11

• Draft comment:
  Updated UserPreference to require non-null UserID for consistency with multi-tenant support.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is purely informative, explaining a change made in the code. It does not provide a suggestion, ask for confirmation, or point out a potential issue.

3. pkg/sqlmigration/021_update_preferences.go:48

• Draft comment:
  Enforcing 'notnull' on UserID improves data integrity, but ensure no existing record has a null value to avoid migration issues.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the author to ensure something, which violates the rule against asking the author to ensure behavior is intended or tested. It doesn't provide a specific code suggestion or ask for a specific test to be written.

4. pkg/types/preference.go:11

• Draft comment:
  UserPreference's UserID now requires a non-null value; verify that all existing data complies with this constraint.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is asking the PR author to verify existing data compliance, which falls under the rule of not asking the author to ensure or verify things. It doesn't provide a specific code suggestion or ask for a test to be written.

Workflow ID: wflow_SNqPjptgINeaBOg1

---

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

[ellipsis-dev]

❌ Changes requested. Incremental review on 395d3b8 in 2 minutes and 0 seconds

More details

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

1. pkg/modules/preference/core/preference.go:89

• Draft comment:
  When GetOrgPreference returns sql.ErrNoRows, orgPreference is nil. Allocate a new instance before setting fields to avoid nil dereference.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

2. pkg/modules/preference/core/preference.go:232

• Draft comment:
  When GetUserPreference returns sql.ErrNoRows, userPreference is nil. Allocate a new instance before setting its fields to avoid nil pointer dereference.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

3. pkg/modules/preference/core/preference.go:232

• Draft comment:
  UpdateUserPreference: When no record is found, userPreference is nil. Allocate a new instance before setting its fields to prevent a runtime panic.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

4. pkg/modules/preference/core/preference.go:125

• Draft comment:
  Typo in comment: 'wherver' should be 'whenever'.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is purely informative and does not provide a code suggestion or ask for a specific change. It simply points out a typo in a comment, which is not critical to the functionality or logic of the code.

5. pkg/modules/preference/core/preference.go:277

• Draft comment:
  Typo in comment: 'wherver' should be 'whenever'.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is purely informative and does not provide a code suggestion or request for a specific action. It simply points out a typo in a comment, which does not affect the functionality of the code.

6. pkg/modules/preference/core/preference.go:125

• Draft comment:
  Typo: In the comment on line 125, 'wherver' should be corrected to 'wherever'.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
  While technically correct, this is an extremely minor typo in a comment that doesn't affect functionality. The rules state not to make purely informative comments and to avoid obvious or unimportant changes. Typos in comments, unless they make the meaning unclear, fall into this category.
  The typo does appear twice in the file, showing it's not just a one-off mistake. Comments are documentation and maintaining their quality could be important.
  Even with multiple instances, this is still just a minor spelling error that doesn't impact code understanding or functionality. The meaning is perfectly clear despite the typo.
  Delete this comment as it violates the rules about not making purely informative comments and avoiding unimportant changes.

7. pkg/modules/preference/core/preference.go:277

• Draft comment:
  Typo: In the comment on line 277, 'wherver' should be corrected to 'wherever'.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While the comment is technically correct about the typo, fixing comment typos is a very minor issue that doesn't affect functionality. The rules state we should not make purely informative comments or comments about obvious/unimportant things. A typo in a comment falls into this category.
  The typo could be seen as reducing code quality and readability. Multiple small issues can add up to make code less professional.
  While code quality is important, a single typo in a comment is too minor to warrant a PR comment. The reviewer could fix this directly or mention it in passing during code review.
  Delete this comment as it points out an extremely minor issue that doesn't warrant a formal PR comment.

Workflow ID: wflow_8NMBYiHq3OScuqiD

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on a27e458 in 1 minute and 49 seconds

More details

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

1. pkg/query-service/app/server.go:151

• Draft comment:
  Multi-tenant preference module is initialized with preferencecore.NewPreferenceCore. Ensure underlying SQLStore and preference store enforce tenant separation and that org/user preference access includes tenant context.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the author to ensure that tenant separation is enforced in the preference module. This falls under asking the author to ensure behavior, which is against the rules. Therefore, this comment should not be approved.

2. pkg/query-service/app/server.go:151

• Draft comment:
  Ensure that the SQLStore passed (serverOptions.SigNoz.SQLStore) properly implements the types.PreferenceStore interface required by NewPreferenceCore(). This instantiation is key for the new multi-tenant preference module; confirm that its behavior is consistent with previous preference handling.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is asking the PR author to confirm the behavior of the SQLStore implementation, which violates the rule against asking for confirmation of intended behavior. It does not provide a specific code suggestion or point out a specific issue that needs to be addressed.

3. pkg/query-service/app/server.go:433

• Draft comment:
  Replace the use of 'fmt.Println("starting private http")' with a structured logging call (e.g. using zap.L().Info) to maintain consistency in logging.
• 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.

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

• Draft comment:
  Verify that the API handler receives and properly uses the newly added 'Preference' field. Consider adding tests for both org and user preference endpoints to ensure the multi-tenant behavior is correctly implemented.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
  The comment violates several rules:

1. It starts with "Verify that..." which is explicitly called out as not useful
2. It's asking the author to confirm their intention and add tests, which is against the rules
3. It's speculative about multi-tenant behavior without clear evidence
4. It's not pointing out a clear required code change
   The comment might be raising valid concerns about testing and proper implementation. Perhaps there are actual issues with how the preference field is used.
   While testing is important, the rules explicitly state not to ask authors to verify or test their changes. If there were specific issues with the preference implementation, those should be called out directly.
   The comment should be deleted as it violates multiple review rules by asking for verification and testing without pointing out specific required changes.

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

• Draft comment:
  This file is quite long; consider refactoring parts of the server setup into smaller modules for improved maintainability and easier debugging.
• 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.

6. pkg/modules/preference/core/preference.go:122

• Draft comment:
  Typo: In the comment at line 122, 'wherver' should be corrected to 'wherever' for clarity.
• 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.

7. pkg/modules/preference/core/preference.go:274

• Draft comment:
  Typo: In the comment at line 274, 'wherver' should be corrected to 'wherever'.
• 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.

8. pkg/query-service/app/http_handler.go:1333

• Draft comment:
  Typographical error: The variable 'apiRrr' appears to be a misspelling. It would be clearer to use 'apiErr' instead.
• 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/query-service/app/http_handler.go:5210

• Draft comment:
  Typographical error: The variable 'errQuriesByName' is misspelled. It should be renamed to 'errQueriesByName' for clarity and consistency.
• 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.

Workflow ID: wflow_NoUYmAmaVaUN30HG

---

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

[ellipsis-dev]

👍 Looks good to me! Incremental review on 4717d6f in 2 minutes and 9 seconds

More details

• Looked at 1016 lines of code in 10 files
• Skipped 0 files when reviewing.
• Skipped posting 13 drafted comments based on config settings.

1. pkg/types/preferencetypes/store.go:8

• Draft comment:
  Consider replacing hardcoded strings (e.g. "boolean", "org", "user") with the corresponding constants defined in the entity file (e.g. PreferenceValueTypeBoolean, OrgAllowedScope, UserAllowedScope) for consistency.
• 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.

2. pkg/types/preferencetypes/entity.go:176

• Draft comment:
  In IsEnabledForScope, the comparison uses allowedScope == strings.ToLower(scope). Consider normalizing both strings (e.g. converting both to lowercase) to avoid case-sensitivity issues.
• 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.

3. pkg/types/preferencetypes/entity.go:125

• Draft comment:
  In IsValidValue, the type assertions for numeric types are handled by checking int64 and float64. Ensure that cases where numbers come in other compatible forms are either explicitly converted or documented.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment suggests ensuring that numeric types are either explicitly converted or documented. This is a request for documentation or confirmation, which violates the rule against asking the PR author to confirm their intention or ensure behavior. Therefore, this comment should not be approved.

4. pkg/types/preferencetypes/store.go:3

• Draft comment:
  The global PreferenceMap is defined here. In a multi-tenant context, ensure that its usage is clearly documented and that tenant-specific overrides (if needed) are handled at a higher layer.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

5. pkg/modules/preference/api.go:24

• Draft comment:
  The API interface and methods (e.g. GetUserPreference, UpdateUserPreference) are clearly defined. Ensure that the routing middleware properly enforces authentication so that these endpoints are protected.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

6. pkg/modules/preference/core/preference.go:67

• Draft comment:
  The JSON encoding of the preference value in UpdateOrgPreference is wrapped with errors.Wrapf. Ensure that the resulting error messages are descriptive enough for troubleshooting an encoding failure.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

7. pkg/modules/preference/usecase.go:9

• Draft comment:
  The Usecase interface is well-structured. Consider adding inline comments for each method to clarify their multi-tenant behavior (org vs user preferences).
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50%
  None

8. pkg/query-service/app/server.go:151

• Draft comment:
  Preference injection via NewAPI in the server is clear. Verify that multi-tenant behaviors and context propagation (org ID, user ID) work as expected in higher layers.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

9. pkg/query-service/app/http_handler.go:4860

• Draft comment:
  Typo: The variable name 'errQuriesByName' appears multiple times. It should be renamed to 'errQueriesByName' to correctly spell 'queries'.
• 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.

10. pkg/query-service/app/http_handler.go:4406

• Draft comment:
  Typo: The variable 'lastestConfig' is misspelled. It should be 'latestConfig' to correctly indicate the most recent configuration.
• 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/query-service/app/http_handler.go:4874

• Draft comment:
  Typo: In the comment, 'experssion' should be corrected to 'expression'.
• 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.

12. pkg/types/preferencetypes/entity.go:163

• Draft comment:
  Typo: The variable name 'valueMisMatchErr' should be spelled as 'valueMismatchErr' (one word, not split into 'MisMatch') for consistency and clarity.
• 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.

13. pkg/types/preferencetypes/entity.go:140

• Draft comment:
  Minor formatting issue: In the error message on line 140, there's an extra space before the comma in 'min: %v , max:%v'. Consider removing the space for consistency (e.g. 'min: %v, max:%v').
• Reason this comment was not posted:
  Comment was on unchanged code.

Workflow ID: wflow_TifVG2LKRl8GY1f2

---

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

[ellipsis-dev]

❌ Changes requested. Incremental review on b46c99f in 1 minute and 56 seconds

More details

• Looked at 418 lines of code in 6 files
• Skipped 0 files when reviewing.
• Skipped posting 7 drafted comments based on config settings.

1. pkg/modules/preference/core/preference.go:73

• Draft comment:
  Potential nil pointer: In UpdateOrgPreference, if GetOrgPreference returns sql.ErrNoRows, orgPreference may be nil before setting its fields. Allocate a new instance instead of using a nil pointer.
• 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.

2. pkg/modules/preference/core/preference.go:198

• Draft comment:
  Potential nil pointer in UpdateUserPreference: When GetUserPreference returns sql.ErrNoRows, userPreference may be nil, so ensure a new instance is allocated before field assignment.
• 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.

3. pkg/modules/preference/core/preference.go:20

• Draft comment:
  Consider validating that the defaultMap parameter is not nil in NewPreference, to prevent potential runtime errors.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
  While nil map access would cause panics, this is a constructor that takes a dependency - validation of dependencies is typically handled at app startup/dependency injection level. The code shows defaultMap is a core dependency used extensively - it would be immediately obvious in testing if it was nil. This feels like defensive programming that adds noise without much value.
  The comment identifies a real potential issue - nil maps do cause panics. And validating constructor params is a good practice.
  However, this is a core dependency that would fail fast in testing. Adding nil checks everywhere creates noise and doesn't provide much practical value in a well-tested codebase.
  The comment should be removed as it suggests defensive programming that adds noise without providing significant value, especially for a core dependency that would be caught immediately in testing.

4. pkg/modules/preference/core/preference.go:78

• Draft comment:
  Potential nil pointer dereference in UpdateOrgPreference: if store.GetOrgPreference returns sql.ErrNoRows, orgPreference may be nil. Consider allocating a new StorableOrgPreference instance before setting its fields.
• 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.

5. pkg/modules/preference/core/preference.go:198

• Draft comment:
  Potential nil pointer dereference in UpdateUserPreference: if store.GetUserPreference returns sql.ErrNoRows, userPreference may be nil. Allocate a new StorableUserPreference instance before setting fields.
• 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.

6. pkg/query-service/app/server.go:152

• Draft comment:
  Injection of multi-tenant default preference map using NewDefaultPreferenceMap is clear. Ensure that this default map remains up-to-date for tenant-specific configurations if needed in the future.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

7. pkg/modules/preference/core/preference.go:15

• Draft comment:
  Using usecase.defaultMap instead of a global PreferenceMap improves modularity and multi-tenancy support. This pattern decouples preference configuration from global state.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

Workflow ID: wflow_rbHLiksqeRgOJfAs

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 868bfe0 in 1 minute and 47 seconds

More details

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

1. ee/query-service/app/api/api.go:60

• Draft comment:
  Good change initializing the multi-tenant preference module using NewDefaultPreferenceMap().
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

2. pkg/modules/preference/core/preference.go:52

• Draft comment:
  For consistency, consider renaming the parameter 'orgId' to 'orgID' in functions like UpdateOrgPreference.
• Reason this comment was not posted:
  Comment was on unchanged code.

3. pkg/modules/preference/core/preference.go:135

• Draft comment:
  Similarly, consider renaming 'userId' to 'userID' in functions like GetUserPreference and UpdateUserPreference for consistency.
• Reason this comment was not posted:
  Comment was on unchanged code.

4. ee/query-service/app/api/api.go:60

• Draft comment:
  Injection of the default preference map via preferencetypes.NewDefaultPreferenceMap() for multi-tenant support looks good.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

5. pkg/modules/preference/core/preference.go:52

• Draft comment:
  Consider using consistent naming for organization and user identifiers (e.g. orgID/userID instead of orgId/userId) across all functions for clarity.
• Reason this comment was not posted:
  Comment was on unchanged code.

6. pkg/modules/preference/core/preference.go:72

• Draft comment:
  In the update functions, if store.GetOrgPreference/GetUserPreference returns sql.ErrNoRows, ensure that the returned pointer is non-nil before dereferencing (e.g. by initializing a new instance) to avoid a potential nil pointer dereference.
• Reason this comment was not posted:
  Comment was on unchanged code.

7. pkg/types/preferencetypes/preference.go:14

• Draft comment:
  Renaming the JSON/DB field from 'PreferenceId' to 'PreferenceID' improves consistency. This change is good.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

8. pkg/modules/preference/core/preference.go:52

• Draft comment:
  Minor issue: The parameter names for organization and user identifiers are inconsistently capitalized (e.g., orgId vs orgID and userId vs userID). Consider standardizing these names across the file for better readability.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

Workflow ID: wflow_BE7PzIwziY4oxfxL

---

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

[ellipsis-dev]

👍 Looks good to me! Incremental review on b4d770c in 48 seconds

More details

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

1. pkg/sqlmigration/021_update_alertmanager.go:145

• Draft comment:
  Ensure OrgReference is defined and imported correctly. This constant should represent the tenant key.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 30% <= threshold 50%
  The comment is asking the author to ensure that OrgReference is defined and imported correctly. This is a request for confirmation, which is against the rules. However, it also provides a specific suggestion that OrgReference should represent the tenant key, which could be useful. Overall, the comment is more about ensuring correctness rather than providing a specific actionable suggestion.

2. pkg/sqlmigration/021_update_alertmanager.go:177

• Draft comment:
  Ensure OrgReference is defined and correctly passed for multi-tenant key. Validate its type and scope.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is asking the PR author to ensure and validate certain aspects of the code, which violates the rule against asking for confirmation or validation. It doesn't provide a specific suggestion or point out a clear issue.

3. pkg/sqlmigration/021_update_alertmanager.go:145

• Draft comment:
  Ensure 'OrgReference' is defined, imported, and its purpose clearly documented. It’s now being passed to UpdatePrimaryKey; verify it properly implements multi-tenant behavior.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is asking the PR author to ensure that OrgReference is defined, imported, and documented, which violates the rule against asking the author to ensure or verify things. It also asks to verify multi-tenant behavior, which is similar to asking for confirmation of intended behavior.

4. pkg/sqlmigration/021_update_alertmanager.go:177

• Draft comment:
  Ensure 'OrgReference' is correctly utilized in this UpdatePrimaryKey call for alertmanager state and that its behavior is as expected for multi-tenant setups.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is asking the PR author to ensure that a specific behavior is correct, which violates the rule against asking for confirmation or verification of intended behavior. It does not provide a specific suggestion or point out a clear issue.

Workflow ID: wflow_llwWIaUK0SUyGyiu

---

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

[ellipsis-dev]

👍 Looks good to me! Incremental review on 71624b4 in 1 minute and 20 seconds

More details

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

1. pkg/sqlmigration/023_update_apdex_ttl.go:128

• Draft comment:
  Good fix: returning the actual error prevents swallowing an error.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply acknowledges a good fix without offering any specific guidance or questions.

2. pkg/sqlmigration/023_update_apdex_ttl.go:175

• Draft comment:
  Return actual error here to avoid hiding problems in transaction.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

3. pkg/sqlmigration/024_update_reset_password.go:90

• Draft comment:
  Properly returning the error helps error propagation instead of swallowing it.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is purely informative and does not provide a specific suggestion or question. It doesn't align with the rules for good comments, as it doesn't ask for a specific change or confirmation of intent.

4. pkg/sqlmigration/023_update_apdex_ttl.go:130

• Draft comment:
  Bug: Return the actual error instead of nil to avoid silently swallowing errors.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

5. pkg/sqlmigration/023_update_apdex_ttl.go:171

• Draft comment:
  Bug: Propagate errors in the TTL migration branch instead of returning nil.
• 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.

6. pkg/sqlmigration/024_update_reset_password.go:90

• Draft comment:
  Bug: Return the encountered error instead of nil to prevent masking migration issues.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

Workflow ID: wflow_3bGsqrM1gA59CXxG

---

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

[vikrantgupta25]

contributes to - https://github.com/SigNoz/platform-pod/issues/583