-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
fix: support multitenancy in dashboards & savedviews #7237
base: main
Are you sure you want to change the base?
Conversation
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! Reviewed everything up to 9e12298 in 2 minutes and 47 seconds
More details
- Looked at
1553
lines of code in22
files - Skipped
0
files when reviewing. - Skipped posting
25
drafted comments based on config settings.
1. pkg/query-service/app/server.go:103
- Draft comment:
Good usage of SQLStore.BunDB() when initializing components like dashboards and explorer. The code properly waits for the reader to be ready before proceeding. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. pkg/sqlmigration/014_update_dashboards.go:53
- Draft comment:
The migration iterates over both 'dashboards' and 'saved_views' tables to add an org_id column. The use of tx.NewAddColumn() is correct. Ensure that further down the behavior for multiple orgIDs is as expected. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. pkg/types/dashboard.go:19
- Draft comment:
Dashboard type now includes OrgID and uses proper bun struct tags. The UpdateSlug and SlugifyTitle functions are well implemented. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
4. pkg/types/savedview.go:12
- Draft comment:
SavedView type includes OrgID and updated field names. The buns struct tags follow convention and ensure consistency. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
5. pkg/query-service/app/integrations/manager.go:318
- Draft comment:
Parsing and constructing dashboard UUID for integration dashboards is clear, using a delimiter '--'. Consider renaming variable names (e.g. 'dId' does not need to be cast through string conversion) for clarity, although functional correctness is maintained. - Reason this comment was not posted:
Confidence changes required:20%
<= threshold50%
None
6. pkg/query-service/app/metricsexplorer/summary.go:159
- Draft comment:
Good use of errgroup for concurrent API calls to populate metric summary details. The merging of dashboards from multi-tenancy is done correctly using user.OrgID. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
7. pkg/signoz/signoz.go:76
- Draft comment:
The initialization of SigNoz with new cache, web, SQLStore and telemetrystore is clear and leverages factory providers correctly. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
8. pkg/sqlmigration/014_update_dashboards.go:42
- Draft comment:
Consider adding comments about the safety of directly updating the dashboards and saved_views tables in production—explain why setting org_id for a single org is acceptable. - Reason this comment was not posted:
Confidence changes required:30%
<= threshold50%
None
9. pkg/types/savedview.go:7
- Draft comment:
The SavedView struct is clearly defined with proper bun tags for ORM mapping. A couple of points: - The embedded TimeAuditable and UserAuditable fields lack explicit JSON tags. Ensure this is intentional (i.e. sensitive audit info should be hidden or exposed as needed).
- The JSON tag for OrgID is 'orgId' while the column tag is 'org_id'; verify that this naming convention is consistent across the API.
- 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%
The comment is speculative and asks for verification rather than pointing out clear issues. The JSON/column tag naming convention point is not a clear error - snake_case for DB columns and camelCase for JSON is a common pattern. Without seeing TimeAuditable/UserAuditable definitions, we can't judge if missing JSON tags are an issue.
I might be wrong about the JSON tags on embedded types - perhaps there's a clear company standard I'm unaware of. The naming convention point could be valid if there's a strict project standard.
Even if there are standards, the comment asks for verification rather than stating clear issues. It's speculative and doesn't provide clear evidence of problems that need fixing.
Delete the comment as it violates the rules against asking for verification/confirmation and making speculative comments without clear evidence of issues.
10. ee/query-service/app/server.go:352
- Draft comment:
Minor typographical inconsistency: In the error message on line 352, 'orgId is missing in the claims' uses a lowercase 'd' while the field is spelled 'OrgID' (with an uppercase 'D'). Consider changing it to 'OrgID is missing in the claims' 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.
11. frontend/src/types/api/dashboard/getAll.ts:34
- Draft comment:
Typo: 'showALLOption' should likely be 'showAllOption' to maintain proper camelCase naming conventions. - 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. frontend/src/types/api/dashboard/getAll.ts:97
- Draft comment:
Typo: 'timePreferance' should be corrected to 'timePreference' to fix the typographical error. - 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/query-service/app/dashboards/model.go:36
- Draft comment:
The comment on line 36 reads 'InitDB sets up setting up the connection pool global variable.' Consider removing the repeated 'setting up' so it reads 'InitDB sets up the connection pool global variable.' - Reason this comment was not posted:
Comment was on unchanged code.
14. pkg/query-service/app/dashboards/model.go:388
- Draft comment:
There is a grammatical error in the comment (line 288) that reads 'check if dashboard is a has a log operator with contains'. Consider rephrasing it to 'check if the dashboard has a log operator with a contains operation' 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.
15. pkg/query-service/app/explorer/db.go:23
- Draft comment:
The comment on InitWithDSN reads "sets up setting up the connection pool global variable." Consider revising it to remove the duplicate phrase, e.g., "Initializes the connection pool global variable." - Reason this comment was not posted:
Comment was on unchanged code.
16. pkg/query-service/app/explorer/db.go:210
- Draft comment:
In the DeleteView function, the error message reads "error in deleting explorer query: %s". For consistency with other functions which mention 'saved view', consider updating this message to "error in deleting saved view: %s". - Reason this comment was not posted:
Comment was on unchanged code.
17. pkg/query-service/app/http_handler.go:4880
- Draft comment:
Typo: The variable 'errQuriesByName' appears in several places. Please correct it 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.
18. pkg/query-service/app/integrations/test_utils.go:53
- Draft comment:
Typo in the instructions: 'Set source attrib on your signals'. Please update 'attrib' to 'attribute' 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.
19. pkg/query-service/app/integrations/test_utils.go:121
- Draft comment:
Typo in the instructions: 'Set source attrib on your signals'. Consider replacing 'attrib' with 'attribute' 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.
20. pkg/query-service/app/metricsexplorer/summary.go:218
- Draft comment:
Typo: 'TimeSeriesTeeMap' appears to be a misspelling. It looks like it should be 'TimeSeriesTreeMap' for consistency with the rest of the code. - 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.
21. pkg/query-service/app/metricsexplorer/summary.go:394
- Draft comment:
Typo: The function name 'getQueryRangeForRelateMetricsList' might be intended to be 'getQueryRangeForRelatedMetricsList' to improve 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.
22. pkg/query-service/tests/integration/signoz_integrations_test.go:385
- Draft comment:
Typo: The error message says 'could not unmarshal apiResponse.Data json into PipelinesResponse' but the data is being unmarshaled into integrations.IntegrationsListResponse. Please update the error message 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.
23. pkg/query-service/tests/integration/signoz_integrations_test.go:401
- Draft comment:
Typo: The error message reads ' could not unmarshal apiResponse.Data json into PipelinesResponse' (with a leading space and 'PipelinesResponse' instead of the appropriate type). Consider aligning it with the correct type or message style. - 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.
24. pkg/query-service/tests/integration/signoz_integrations_test.go:357
- Draft comment:
Typo: The comment says 'Integration dashboards should not longer appear in dashboard list after uninstallation'. Consider rewording it to 'Integration dashboards should no longer appear in the dashboard list after uninstallation'. - 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.
25. pkg/query-service/tests/integration/signoz_integrations_test.go:472
- Draft comment:
Minor formatting issue: The error message ' could not unmarshal apiResponse.Data json into dashboards' has an unnecessary leading space. Please remove it for a cleaner message. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_XsBDNlXUwNWPTMoP
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
savedViews, err := GetViews() | ||
// get single org ID from db | ||
var orgIDs []string | ||
err := db.NewSelect().Model((*types.Organization)(nil)).Column("id").Scan(ctx, &orgIDs) |
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.
I am doing this since I wan't to use the existing GetViews. But we will have to look into telemetry collection all together. Keeping a separete issue for this.
@nityanandagohain please give me 2ish days. I want to get this #7222 merged which helps with the provider config. |
@nityanandagohain that PR was merged early morning today. Can you update this? |
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 4cc6ebe in 2 minutes and 2 seconds
More details
- Looked at
5683
lines of code in61
files - Skipped
0
files when reviewing. - Skipped posting
27
drafted comments based on config settings.
1. pkg/types/alertmanagertypes/route.go:8
- Draft comment:
Consider extracting the inline YAML unmarshalling helper (func(i interface{}) error { return nil }) into a named helper function to reduce duplication and improve readability. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
2. pkg/types/alertmanagertypes/route.go:31
- Draft comment:
The function NewRouteFromReceiver relies on a preconfigured noRuleIDMatcher. Double-check that this default matcher meets all use‐cases; consider documenting the rationale more explicitly. - Reason this comment was not posted:
Confidence changes required:30%
<= threshold50%
None
3. pkg/query-service/rules/db.go:162
- Draft comment:
Consider using parameterized queries (e.g. placeholders) rather than fmt.Sprintf in GetStoredRule to improve safety and readability. - 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/sqlmigration/014_add_alertmanager.go:50
- Draft comment:
Error handling via substring matching ("no such column") is brittle; consider using error codes or type assertions. - 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 the comment raises a valid point about error handling best practices, this is in a migration file where string matching errors is actually quite common. The error message comes from the database layer and getting proper error codes may not be straightforward. The current approach, while not ideal, is pragmatic for a migration script.
The comment suggests a better practice but doesn't consider that this is a migration file where string matching DB errors is common. Also, it's not clear if better alternatives (error codes/types) are actually available from the DB layer.
While better error handling would be nice, the current approach is acceptable for a migration script and changing it may add unnecessary complexity.
The comment should be deleted as it suggests a change that would add complexity without significant benefit in this specific migration context.
5. pkg/signoz/config.go:110
- Draft comment:
Consider using a logger instead of fmt.Println for deprecation messages to ensure consistency in logging. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
6. pkg/types/alertmanagertypes/receiver.go:33
- Draft comment:
Using JSON->YAML round-trip to populate defaults is clever, but be aware this may be costly; consider a custom initialization if performance is critical. - Reason this comment was not posted:
Confidence changes required:40%
<= threshold50%
None
7. pkg/types/alertmanagertypes/route.go:8
- Draft comment:
Ensure the route's UnmarshalYAML call successfully fills defaults; review behavior when route is provided non-nil. - Reason this comment was not posted:
Confidence changes required:30%
<= threshold50%
None
8. conf/example.yaml:113
- Draft comment:
Typo: 'exahustive' should be replaced with 'exhaustive' in the comment at line 113. - 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 typo is real, it's in a documentation comment and doesn't affect functionality. The meaning is still clear despite the typo. According to the rules, we should not make purely informative comments that don't require code changes. While this suggests a change, it's an extremely minor one that doesn't impact code quality or maintainability.
The typo could potentially confuse users looking at the documentation or trying to search for terms in the codebase. It's also part of newly added code.
While the typo is in new code, it's still just a minor documentation issue that doesn't impact functionality or maintainability. The link to the upstream documentation still works, and the meaning is clear.
Delete the comment. While it points out a real typo, it's too minor of an issue to warrant a PR comment according to our rules about not making purely informative comments.
9. ee/query-service/app/api/auth.go:288
- Draft comment:
Typographical error: In the receiveSAML function, the debug log on line 288 uses the tag "[receiveML]". It should likely be "[receiveSAML]" 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.
10. ee/query-service/rules/manager.go:40
- Draft comment:
Typographical error: The comment 'create ch rule task for evalution' should be changed to 'create ch rule task for evaluation'. - 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. ee/query-service/rules/manager.go:61
- Draft comment:
Typographical error: The comment 'create promql rule task for evalution' should be corrected to 'create promql rule task for evaluation'. - 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. ee/query-service/rules/manager.go:81
- Draft comment:
Typographical error: The comment 'create anomaly rule task for evalution' should be updated to 'create anomaly rule task for evaluation'. - 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. ee/query-service/rules/manager.go:175
- Draft comment:
Typographical error: In the error message on line 175, 'ruletype' should be written as 'rule type' to improve 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.
14. pkg/query-service/app/clickhouseReader/options.go:98
- Draft comment:
Typographical error: The comment on line 98 incorrectly uses 'Connecto' instead of 'Connector'. Please update the comment to match the correct terminology. - Reason this comment was not posted:
Comment was on unchanged code.
15. pkg/query-service/app/http_handler.go:0
- Draft comment:
Typographical error: The variable name 'errQuriesByName' is misspelled in several locations. It should be corrected to 'errQueriesByName' for better 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.
16. pkg/query-service/app/http_handler.go:1298
- Draft comment:
Typographical error: In the testRule function, the variable 'apiRrr' is used. It seems to be a typo and should be renamed to 'apiErr' for consistency with other error variables. - 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.
17. pkg/query-service/app/parser.go:757
- Draft comment:
Consider rephrasing the comment on offsetting the start time. 'Offset by two window periods for better results' might be clearer than 'offset by two windows periods for start for better results'. - 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 does suggest a clearer wording, the review rules state we should not make purely informative comments. The existing comment, while not perfect, is still understandable and the suggested change is minor. The comment doesn't point out a real code issue that needs fixing.
The comment does identify a legitimate clarity issue in the code comment. Clear documentation is important for maintainability.
However, the review rules explicitly state not to make purely informative comments. The existing comment is functional even if not optimal.
The comment should be deleted as it is purely informative and doesn't identify a required code change.
18. pkg/query-service/auth/auth.go:498
- Draft comment:
Typographical error: The function call 'utils.GeneratePassowrd()' appears to have a typo. It should likely be 'utils.GeneratePassword()'. - 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.
19. pkg/query-service/auth/auth.go:257
- Draft comment:
Typographical error: The error message "failed to org id from context" seems to be missing a word. Consider changing it to "failed to extract org id from context" 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.
20. pkg/query-service/constants/constants.go:16
- Draft comment:
Typo: Consider correcting the comment on line 16 from "Address to server internal services like alert manager" to "Address to serve internal services like alert manager". - 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.
21. pkg/query-service/model/v3/v3.go:1355
- Draft comment:
Typo: The comment on line 1355 says 'priortize the last point'; consider changing 'priortize' to 'prioritize' 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.
22. pkg/query-service/rules/api_params.go:300
- Draft comment:
Typographical note: In the GettableRule struct, the JSON keys for CreatedAt, CreatedBy, UpdatedAt, and UpdatedBy are currently 'createAt', 'createBy', 'updateAt', and 'updateBy'. It might be clearer and more consistent to use 'createdAt', 'createdBy', 'updatedAt', and 'updatedBy' (note the missing 'd') if this is not intentional. - 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.
23. pkg/query-service/rules/base_rule.go:69
- Draft comment:
Typo: Consider changing "dont" to "don't" in the comment above the sendUnmatched field for better readability. - 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.
24. pkg/query-service/rules/base_rule.go:73
- Draft comment:
Typo: Consider changing "irresepective" to "irrespective" in the comment above the sendAlways field. - 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.
25. pkg/query-service/rules/manager.go:172
- Draft comment:
Typo in comment: 'create ch rule task for evalution' should be corrected to 'create ch rule task for evaluation'. - 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.
26. pkg/query-service/rules/manager.go:193
- Draft comment:
Typo in comment: 'create promql rule task for evalution' should be corrected to 'create promql rule task for evaluation'. - 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.
27. pkg/types/alertmanagertypes/config.go:230
- Draft comment:
Typographical inconsistency: In the GetReceiver function, the error message says "channel with name %q not found" and uses ErrCodeAlertmanagerChannelNotFound, but the rest of the code refers to these as 'receivers' and the defined error code is ErrCodeAlertmanagerConfigNotFound. Please correct the error message and error code 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.
Workflow ID: wflow_fCI38fnNkQhcqRGR
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 d355c02 in 57 seconds
More details
- Looked at
78
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
7
drafted comments based on config settings.
1. pkg/query-service/utils/testutils.go:51
- Draft comment:
Ensure that switching to NewUpdateDashboardAndSavedViewsFactory correctly reflects the multi-tenancy changes. Confirm that any legacy dashboard-only migration code is removed. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to ensure and confirm certain changes, which violates the rule against asking for confirmation or ensuring behavior. It doesn't provide a specific code suggestion or ask for a specific test to be written.
2. pkg/signoz/provider.go:62
- Draft comment:
Double-check that adding NewUpdateDashboardAndSavedViewsFactory in the migration providers fully supports tenant scoping for dashboards and saved views. Verify that telemetry code is updated accordingly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to double-check and verify certain aspects of their change, which is against the rules. It does not provide a specific code suggestion or point out a specific issue that needs addressing.
3. pkg/sqlmigration/015_update_dashboards_savedviews.go:60
- Draft comment:
Consider adding error context or logging for migration failures and note in comments that the Down method is a no-op. This ensures clarity during rollback scenarios. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
4. pkg/query-service/utils/testutils.go:51
- Draft comment:
The migration factory call has been updated to include saved views support. Ensure the tests validate multi-org scenarios in addition to the single-org fallback. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
5. pkg/signoz/provider.go:62
- Draft comment:
The SQL migration provider now includes the update for dashboards and saved views, but the internal migration name 'update_group' is not very descriptive. Consider renaming it for clarity. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
6. pkg/sqlmigration/015_update_dashboards_savedviews.go:52
- Draft comment:
In the migration Up function, org_id values are set only when exactly one organization exists. Confirm that this behavior is intended in multi-org deployments. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
7. pkg/sqlmigration/015_update_dashboards_savedviews.go:40
- Draft comment:
Consider wrapping errors with additional context in the transaction start and column operations to aid future debugging. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_u2D94ZEwcb4nnsX3
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
@@ -68,6 +68,15 @@ func (n *NamedMap[T]) Add(factory T) (err error) { | |||
return nil | |||
} | |||
|
|||
func (n *NamedMap[T]) AddAll(factories ...T) (err error) { |
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.
not using now, let me know if we should remove it.
done @grandwizard28 |
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 4abc7cf in 1 minute and 45 seconds
More details
- Looked at
444
lines of code in6
files - Skipped
0
files when reviewing. - Skipped posting
10
drafted comments based on config settings.
1. pkg/query-service/auth/rbac.go:80
- Draft comment:
Typo: Change "atleast" to "at least" in the password validation error message. - Reason this comment was not posted:
Comment was on unchanged code.
2. pkg/query-service/auth/rbac.go:13
- Draft comment:
Type 'Group' is defined but never used. Consider removing it if it's not 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.
3. pkg/query-service/auth/rbac.go:80
- Draft comment:
Typo in error message: use 'at least' (with a space) instead of 'atleast'. - Reason this comment was not posted:
Comment was on unchanged code.
4. pkg/query-service/auth/rbac.go:79
- Draft comment:
The variable 'minimumPasswordLength' is used but not defined in this file. Ensure it is imported or defined for clarity. - Reason this comment was not posted:
Comment was on unchanged code.
5. pkg/query-service/auth/rbac.go:32
- Draft comment:
In setGroupId, err is wrapped using err.Err. Confirm that the error returned by dao.DB().GetGroupByName has an 'Err' field; consider using standard error wrapping (e.g. %w) if appropriate. - 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/dashboards/model.go:282
- Draft comment:
There's a minor typo in the comment on line 282: 'check if dashboard is a has a log operator with contains' should be rephrased to something like 'check if the dashboard has a log operator with a contains clause' 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/query-service/app/http_handler.go:1480
- Draft comment:
Typographical Issue: The variable name 'errQuriesByName' appears multiple times in the file. It is likely intended to be spelled 'errQueriesByName' for better clarity and consistency. Please consider correcting the spelling. - 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/metricsexplorer/summary.go:400
- Draft comment:
It appears that the function name 'getQueryRangeForRelateMetricsList' may be a typographical error. For consistency with the rest of the code (which refers to related metrics), consider renaming it to 'getQueryRangeForRelatedMetricsList'. - 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/auth/rbac.go:80
- Draft comment:
Typographical error: In the error message, please change 'Password should be atleast %d characters.' to 'Password should be at least %d characters.' - Reason this comment was not posted:
Comment was on unchanged code.
10. pkg/query-service/auth/rbac.go:26
- Draft comment:
Grammatical typo in the comment for InitAuthCache: consider updating the comment from 'reads the DB and initialize the auth cache' to 'reads the DB and initializes the auth cache.' - 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_4E9njf3NpEQhYBuY
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 2cc67cd in 2 minutes and 21 seconds
More details
- Looked at
179
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
10
drafted comments based on config settings.
1. pkg/query-service/app/http_handler.go:1060
- Draft comment:
Inconsistent error handling for missing claims: standardize responses for authentication failures. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. pkg/query-service/app/http_handler.go:1064
- Draft comment:
Wrap raw error in model.ApiError for consistent error formatting. - 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/query-service/app/http_handler.go:2138
- Draft comment:
Avoid using context.Background() in handler functions; use r.Context() for proper cancellation propagation. - 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/http_handler.go:2160
- Draft comment:
In functions like editUser, use r.Context() instead of context.Background() to ensure proper propagation of the request context. - 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/query-service/app/http_handler.go:1057
- Draft comment:
Good: getDashboards now extracts the tenant’s OrgID via ClaimsFromContext and passes it to dashboards.GetDashboards, correctly enforcing multi‐tenancy for dashboard listing. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
6. pkg/query-service/app/http_handler.go:4586
- Draft comment:
Nice: The saved views handlers (create, get, update, delete) now use ClaimsFromContext to retrieve OrgID and pass it to explorer functions, ensuring tenant isolation. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
7. pkg/query-service/app/http_handler.go:1124
- Draft comment:
Suggestion: There is a mix of error handling methods – unauthenticated errors use render.Error while other errors use RespondError. Consider standardizing error responses 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.
8. pkg/query-service/app/http_handler.go:1090
- Draft comment:
Note: The tag filtering logic in getDashboards uses an intersection algorithm over dashboard indices. Ensure this approach scales well as the number of dashboards increases. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
9. pkg/query-service/app/http_handler.go:4628
- Draft comment:
Overall, multi‐tenancy for dashboards and saved views is properly enforced by passing the OrgID from the claims into the explorer/dashboard functions. Make sure to add tests for scenarios with different tenant IDs. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
10. pkg/query-service/app/http_handler.go:1319
- Draft comment:
Typo: The variable 'apiRrr' in the testRule function appears to be a typo. It should be renamed to 'apiErr' 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_IwyhA5Ze0JmWrgDF
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Fixes https://github.com/SigNoz/platform-pod/issues/461
Fixes https://github.com/SigNoz/platform-pod/issues/462
Radius
DASHBOARDS_PATH
Note:
Important
Add multitenancy support for dashboards and saved views, including database schema updates and refactoring to handle organization-specific data.
OrgID
indashboard.go
,server.go
, andsummary.go
.GetDashboards
,CreateDashboard
,UpdateDashboard
, andDeleteDashboard
functions to includeOrgID
.GetViews
,CreateView
,UpdateView
, andDeleteView
inexplorer/db.go
to handleOrgID
.OrgID
column todashboards
andsaved_views
tables in015_update_dashboards_savedviews.go
.updateDashboardAndSavedViews
to addOrgID
to existing records if only one organization exists.Dashboard
andSavedView
structs intypes/dashboard.go
andtypes/savedview.go
to includeOrgID
.signoz_integrations_test.go
to verify multitenancy behavior.DASHBOARDS_PATH
and related provisioning code.auth/rbac.go
to useClaims
for authorization checks.This description was created by
for 2cc67cd. It will automatically update as commits are pushed.