-
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
Feat: aws integrations: dashboards #7058
base: main
Are you sure you want to change the base?
Conversation
5f1d965
to
8e0a7af
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Reviewed everything up to 8e0a7af in 1 minute and 48 seconds
More details
- Looked at
3702
lines of code in9
files - Skipped
2
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. pkg/query-service/app/integrations/builtin.go:212
- Draft comment:
Consider sanitizing the relative file path in readFileIfUri to prevent directory traversal, even if embedded FS limits risk. - 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/query-service/app/integrations/builtin.go:253
- Draft comment:
The toPromMetricName helper cleanly replaces non-alphanumeric characters; consider adding a comment that explains why no length truncation is performed. - Reason this comment was not posted:
Confidence changes required:30%
<= threshold50%
None
Workflow ID: wflow_Qi4O45v5iTXmOMga
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.
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 7982921 in 1 minute and 14 seconds
More details
- Looked at
34
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. pkg/query-service/tests/integration/signoz_integrations_test.go:12
- Draft comment:
Import for cloudintegrations package added; ensure consistency with naming conventions. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. pkg/query-service/tests/integration/signoz_integrations_test.go:558
- Draft comment:
New cloudIntegrationsController is initialized with proper error check; verifies that APIHandler now includes CloudIntegrations. Looks good. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. pkg/query-service/tests/integration/signoz_integrations_test.go:15
- Draft comment:
New import for cloudintegrations added. Ensure you add tests covering cloud integration dashboard endpoints if not done already. - Reason this comment was not posted:
Marked as duplicate.
4. pkg/query-service/tests/integration/signoz_integrations_test.go:561
- Draft comment:
CloudIntegrationsController is correctly instantiated and injected. Consider adding dedicated integration tests for cloud dashboards to cover its API routes. - 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 adding tests is generally good, this comment violates several rules: 1) It's purely informative rather than pointing out a required code change 2) It's speculative - we don't know if cloud dashboard tests are actually needed 3) It's asking the author to add something rather than fixing an actual issue 4) Understanding whether these tests are needed would require more context about the cloud integrations feature.
The comment does identify a potential gap in test coverage, which could be important for code quality. Maybe cloud dashboard tests are critical for this feature.
While test coverage is important, this comment doesn't identify a clear issue that needs fixing. If cloud dashboard tests were truly required, they should have been part of the acceptance criteria or requirements, not suggested as an afterthought in a PR comment.
Delete this comment. While it makes a reasonable suggestion, it violates our rules about making purely informative comments and speculative suggestions rather than pointing out actual issues that need fixing.
Workflow ID: wflow_cjoGx9Sw07NiFrnd
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 5634951 in 1 minute and 13 seconds
More details
- Looked at
45
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. pkg/query-service/app/cloudintegrations/controller.go:129
- Draft comment:
Consider using net/url’s URL and Values to construct the query string instead of manual string concatenation. - Reason this comment was not posted:
Marked as duplicate.
2. pkg/query-service/app/cloudintegrations/controller.go:134
- Draft comment:
Using a map for URL query parameters can lead to non-deterministic ordering. Consider using a sorted list or url.Values for consistent ordering. - Reason this comment was not posted:
Marked as duplicate.
3. pkg/query-service/app/cloudintegrations/controller.go:127
- Draft comment:
agentVersion is hardcoded as 'latest'; consider parameterizing via configuration for future flexibility. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. pkg/query-service/app/cloudintegrations/controller.go:129
- Draft comment:
Consider using url.URL and url.Values to build the connection URL instead of manual string concatenation for better robustness. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
5. pkg/query-service/app/cloudintegrations/controller.go:134
- Draft comment:
Building query parameters via map iteration can lead to inconsistent ordering and may inadvertently expose sensitive keys (e.g. API keys) in the URL. Consider using url.Values to properly encode parameters and review the security implications of including credentials in URL query parameters. - 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%
The map iteration order concern is valid - Go maps have random iteration order. However, this is a CloudFormation URL where parameter order doesn't matter. The security concern about exposing keys in URLs is valid, but this appears to be for AWS CloudFormation which requires parameters to be passed this way. The keys are properly escaped using url.QueryEscape().
I could be wrong about CloudFormation requirements - maybe there's a more secure way to pass sensitive parameters. The random map iteration, while not impacting functionality, could make debugging harder.
CloudFormation quick create URLs are designed to work this way - the parameters need to be in the URL. The random iteration order doesn't affect functionality since URL parameter order doesn't matter.
The comment raises valid concerns but doesn't offer actionable solutions given the constraints of CloudFormation quick create URLs. The current implementation properly escapes parameters and works as intended.
Workflow ID: wflow_80kD6ugzud1vTWIA
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Summary
Related Issues / PR's
Contributes to https://github.com/SigNoz/engineering-pod/issues/2023
Important
Adds AWS cloud integration dashboards for EC2 and RDS, updates API to include these dashboards, and enhances tests for new functionalities.
http_handler.go
.controller.go
.aws/ec2/integration.json
andaws/rds/integration.json
.AvailableDashboards
andGetDashboardById
incontroller.go
for handling cloud dashboards.validateServiceDefinition
inavailableServices.go
to check for duplicate dashboard IDs.signoz_integrations_test.go
to test new dashboard functionalities..jpeg
,.jpg
, and.png
files inreadFileIfUri()
inbuiltin.go
.This description was created by
for 5634951. It will automatically update as commits are pushed.