-
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 integration support for api gateway #7232
base: main
Are you sure you want to change the base?
Conversation
ab308a5
to
12eb23d
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 12eb23d in 2 minutes and 38 seconds
More details
- Looked at
1659
lines of code in4
files - Skipped
2
files when reviewing. - Skipped posting
8
drafted comments based on config settings.
1. pkg/query-service/app/cloudintegrations/serviceDefinitions/aws/api-gateway/assets/dashboards/overview.json:3
- Draft comment:
Consider using design tokens or predefined color constants for SVG fill colors (#4D27A8, #A166FF) instead of hardcoding values to maintain design consistency. - 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%
This appears to be an AWS service icon that's likely copied directly from AWS's own assets. The colors are probably meant to match AWS's branding. Changing these to design tokens wouldn't make sense since they need to match AWS's specific brand colors. Additionally, this is in a JSON configuration file, not a React component where design tokens would typically be used.
The comment does align with the general best practice of using design tokens. Perhaps there's a way to store AWS brand colors as tokens for consistency across the app?
While design tokens are good practice generally, this is a special case of an AWS service icon that should match AWS branding exactly. Creating tokens for third-party brand colors would add unnecessary complexity.
The comment should be deleted. The colors are part of AWS's brand identity for their API Gateway service and should remain as hardcoded values.
2. pkg/query-service/app/cloudintegrations/serviceDefinitions/aws/rds/overview.md:1
- Draft comment:
Ensure the change from 'Monitor RDS Postgres with SigNoz' to 'Monitor RDS with SigNoz' is intentional and covers all RDS types as expected. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to confirm their intention regarding a change in wording. This violates the rule against asking for confirmation of intention.
3. pkg/query-service/app/cloudintegrations/serviceDefinitions/aws/api-gateway/assets/dashboards/overview.json:3
- Draft comment:
Avoid hardcoding color values in the inline SVG. Use design tokens or predefined color constants to ensure design consistency. - 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%
This is a tricky case. While using design tokens is generally good practice, this appears to be an AWS API Gateway icon SVG that's likely copied directly from AWS's assets. The colors are part of AWS's branding. Changing these to use design tokens would mean deviating from AWS's official icon. Additionally, this is in a JSON file containing dashboard configuration, not in React/frontend code where design tokens would typically be used.
The comment raises a valid general principle about design consistency. However, am I being too lenient by ignoring it just because it's an AWS icon?
Even if design tokens are good practice in general, modifying official AWS brand assets would be inappropriate. This isn't a case where we're defining our own UI elements.
The comment should be deleted. While using design tokens is good practice, this is an official AWS icon where maintaining brand consistency is more important.
4. pkg/query-service/app/cloudintegrations/serviceDefinitions/aws/rds/overview.md:1
- Draft comment:
The title update to 'Monitor RDS with SigNoz' looks intentional. Ensure that this change aligns with the supported RDS metrics. - 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 alignment with supported RDS metrics, which is a form of asking for confirmation or verification. It doesn't provide a specific suggestion or point out a clear issue with the code.
5. pkg/query-service/app/cloudintegrations/serviceDefinitions/aws/api-gateway/assets/dashboards/overview.json:72
- Draft comment:
Typo: Consider renaming 'showALLOption' to 'showAllOption' for consistency. - 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 suggestion would improve naming consistency by using proper camelCase, this seems like a very minor stylistic issue. The property name is used consistently within this file (both Account and Region use the same casing). Without seeing other files or understanding if this is part of a larger API/schema, I can't be certain if changing this would break compatibility.
The comment identifies a real inconsistency in casing style. Perhaps there are other files that use this property name that would make this change more important for overall codebase consistency.
However, without seeing evidence that this is part of a larger pattern or requirement, this seems like an overly nitpicky comment about casing in a new file. The current casing is at least used consistently within this file.
Delete the comment as it suggests a minor stylistic change without strong evidence that it's necessary or part of a broader requirement.
6. pkg/query-service/app/cloudintegrations/serviceDefinitions/aws/api-gateway/integration.json:143
- Draft comment:
Typo: Consider using 'Account ID' instead of 'Account Id' for consistency with common acronym capitalizations. - 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 technically correct that "ID" is commonly capitalized in technical contexts, this is just a display name that will be shown to users. The current capitalization "Id" is still readable and understandable. The comment is about a very minor stylistic issue that doesn't affect functionality. Looking at the rules, we should not make purely informative comments or comments about unimportant issues.
The capitalization inconsistency could potentially cause confusion if other parts of the UI use "ID". Also, following AWS's own style guide might be important for consistency.
While consistency is good, this is too minor of an issue to warrant a comment. It doesn't affect functionality and the meaning is clear either way.
Delete the comment as it addresses a very minor stylistic issue that doesn't significantly impact usability or functionality.
7. pkg/query-service/app/cloudintegrations/serviceDefinitions/aws/api-gateway/overview.md:3
- Draft comment:
Consider hyphenating 'out of the box' to 'out-of-the-box' for clarity and consistency. - 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 rules focus on code quality and implementation issues, not documentation style. This is a very minor stylistic suggestion that doesn't impact functionality or understanding. Both forms (with or without hyphens) are commonly used and understood. This seems like an unnecessary nitpick.
Documentation clarity and consistency could be important for maintaining professional standards. Maybe inconsistent hyphenation across docs could be confusing?
While consistency is good, this level of nitpicking about optional hyphens doesn't warrant a PR comment. The meaning is clear either way.
Delete this comment as it's an extremely minor stylistic suggestion about documentation that doesn't impact functionality or understanding.
8. pkg/query-service/app/cloudintegrations/serviceDefinitions/aws/rds/overview.md:3
- Draft comment:
Consider hyphenating 'out of the box dashboard' to 'out-of-the-box dashboard' to improve readability and ensure consistency. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_wkEmeb7jGqOsIr0a
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.
...ice/app/cloudintegrations/serviceDefinitions/aws/api-gateway/assets/dashboards/overview.json
Outdated
Show resolved
Hide resolved
...ice/app/cloudintegrations/serviceDefinitions/aws/api-gateway/assets/dashboards/overview.json
Show resolved
Hide resolved
12eb23d
to
c3e147c
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.
👍 Looks good to me! Incremental review on c3e147c in 1 minute and 29 seconds
More details
- Looked at
65
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/serviceDefinitions/aws/api-gateway/assets/dashboards/overview.json:250
- Draft comment:
The query for latency (previously 'aws_ApiGateway_Latency_max') is now an empty string. Confirm this is intentional for ALB integration or add a proper query placeholder. - 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 asks for confirmation that the change is intentional, which violates the rule "Do NOT ask the PR author to confirm their intention". Additionally, the comment is speculative by suggesting it might be related to ALB integration. The rules state we should not make speculative comments. The change is clearly visible in the diff and appears to be a deliberate pattern repeated multiple times.
The empty queries could potentially break dashboard functionality, so maybe this is a legitimate concern worth raising?
While dashboard functionality is important, asking for confirmation of intention is explicitly against the rules. If there was a clear technical issue, it should be stated directly rather than asking for confirmation.
The comment should be deleted as it violates the rules against asking for confirmation of intent and making speculative suggestions.
2. pkg/query-service/app/cloudintegrations/serviceDefinitions/aws/api-gateway/assets/dashboards/overview.json:250
- Draft comment:
Several panels now have an empty 'query' field (was 'aws_ApiGateway_Latency_max'). If this is intentional (to disable the SQL/promql query), consider removing the property or adding a comment. Otherwise, data may not be fetched. - 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/cloudintegrations/serviceDefinitions/aws/api-gateway/assets/dashboards/overview.json:1438
- Draft comment:
File missing a trailing newline; please add one for consistency. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =30%
<= threshold50%
The comment is asking for a trailing newline for consistency. This is a common code style guideline, but it doesn't provide a specific code suggestion or address a potential issue with the code's functionality. It seems to be more of a stylistic preference.
4. pkg/query-service/app/cloudintegrations/serviceDefinitions/aws/api-gateway/assets/dashboards/overview.json:72
- Draft comment:
Typo: The property 'showALLOption' appears to be misspelled. Consider renaming it to 'showAllOption' 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.
5. pkg/query-service/app/cloudintegrations/serviceDefinitions/aws/api-gateway/assets/dashboards/overview.json:316
- Draft comment:
Typo: The property 'timePreferance' is misspelled. It should be corrected to 'timePreference' to avoid confusion. - 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_sGJEYyL73VsH5V35
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Summary
Adds support for Application Load Balancer as an AWS integration service
Related Issues / PR's
Contributes to https://github.com/SigNoz/engineering-pod/issues/2023
Important
Adds AWS API Gateway integration support with new dashboard and configuration files, and updates RDS monitoring documentation.
overview.json
for API Gateway dashboard inpkg/query-service/app/cloudintegrations/serviceDefinitions/aws/api-gateway/assets/dashboards/
.integration.json
for API Gateway integration configuration inpkg/query-service/app/cloudintegrations/serviceDefinitions/aws/api-gateway/
.overview.md
for API Gateway monitoring description inpkg/query-service/app/cloudintegrations/serviceDefinitions/aws/api-gateway/
.overview.md
inaws/rds
to generalize RDS monitoring description.This description was created by
for c3e147c. It will automatically update as commits are pushed.