Skip to content
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

chore: aws integration UI events #7172

Merged
merged 16 commits into from
Feb 26, 2025
Merged

Conversation

raj-k-singh
Copy link
Collaborator

@raj-k-singh raj-k-singh commented Feb 22, 2025

Summary

Adds logging of UI events for interesting AWS integrations interactions

Related Issues / PR's

contributes to https://github.com/SigNoz/engineering-pod/issues/2023


Important

Add logging for AWS integration UI events and update modal title in CloudAccountSetupModal.tsx.

  • Logging:
    • Add logEvent calls for AWS integration UI events in AccountActions.tsx, AccountSettingsModal.tsx, and ServiceDetails.tsx.
    • Log events for actions like viewing accounts, starting connection attempts, and saving settings.
  • Behavior:
    • Update CloudAccountSetupModal.tsx title from 'AWS Webservice Integration' to 'AWS Integration'.
  • Timeouts:
    • Increase errorTimeout in RegionForm.tsx from 5 to 10 minutes.

This description was created by Ellipsis for c3e1544. It will automatically update as commits are pushed.

@github-actions github-actions bot added the chore label Feb 22, 2025
@raj-k-singh raj-k-singh marked this pull request as ready for review February 23, 2025 08:14
@raj-k-singh raj-k-singh requested a review from YounixM as a code owner February 23, 2025 08:14
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 ff209eb in 2 minutes and 25 seconds

More details
  • Looked at 323 lines of code in 10 files
  • Skipped 0 files when reviewing.
  • Skipped posting 10 drafted comments based on config settings.
1. frontend/src/hooks/integration/aws/useAccountSettingsModal.ts:113
  • Draft comment:
    Avoid mutating arrays with sort(). Use a non-mutative approach like [...selectedRegions].sort() to compare arrays.
  • 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. frontend/src/container/CloudIntegrationPage/HeroSection/components/CloudAccountSetupModal.tsx:120
  • Draft comment:
    Avoid inline style usage; move 'display: none' into a CSS class for cancelButtonProps.
  • 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. frontend/src/container/CloudIntegrationPage/ServicesSection/ConfigureServiceModal.tsx:137
  • Draft comment:
    Avoid inline styles for cancelButtonProps; use a CSS class instead of inline style 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.
4. frontend/src/container/CloudIntegrationPage/HeroSection/components/CloudAccountSetupModal.tsx:120
  • Draft comment:
    Avoid using inline styles (e.g. in cancelButtonProps) in modal config. Extract these styles into a CSS class to maintain design consistency and ease theming.
  • 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. frontend/src/hooks/integration/aws/useIntegrationModal.ts:121
  • Draft comment:
    When using window.open, include 'noopener,noreferrer' in the feature string to improve security. For example: window.open(url, '_blank', 'noopener,noreferrer').
  • Reason this comment was not posted:
    Comment was on unchanged code.
6. frontend/src/container/CloudIntegrationPage/HeroSection/components/AccountSettingsModal.tsx:75
  • Draft comment:
    There appears to be a repeated word in the class name at line 75: 'account-settings-modal__body-regions-switch-switch '. Please verify if the duplication of 'switch' is intentional. If not, consider renaming it to a more concise class name (e.g., 'account-settings-modal__body-regions-switch').
  • 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. frontend/src/container/CloudIntegrationPage/HeroSection/components/CloudAccountSetupModal.tsx:130
  • Draft comment:
    Typographical error: The AWS service should be referred to as 'CloudFormation' (one word) instead of 'Cloud Formation Template'. Please update the text for consistency with AWS 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.
8. frontend/src/container/CloudIntegrationPage/ServicesSection/ConfigureServiceModal.tsx:140
  • Draft comment:
    Minor issue: The class name provided to the rootClassName prop in the SignozModal component (line 140) begins with an extra space. Please remove the leading whitespace to maintain 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.
9. frontend/src/container/CloudIntegrationPage/constants.ts:18
  • Draft comment:
    Typo: The event string for SERVICE_SETTINGS_OPENED ('AWS Integration: Service: Settings: Opened') uses an extra colon after 'Settings'. To maintain consistency with similar events (e.g., ACCOUNT_SETTINGS_VIEWED uses 'Settings Viewed'), consider removing the extra colon.
  • 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 identifies a real inconsistency in the string format. However, this is a very minor formatting issue in telemetry event strings. The strings are still functional and clear either way. The comment doesn't point out a bug or significant code quality issue. It's more of a nitpick about string formatting consistency.
    The inconsistency could make the codebase slightly harder to maintain if different formats are used throughout. It might indicate a lack of standardization in event naming.
    While consistency is good, this is an extremely minor issue. The event strings are still clear and functional. This kind of nitpicky comment about string formatting doesn't meet the bar of "clearly a code change required."
    Delete this comment. While it points out a real inconsistency, it's too minor of an issue and doesn't meet the criteria of requiring a clear code change.
10. frontend/src/container/CloudIntegrationPage/constants.ts:19
  • Draft comment:
    Typo: The event string for SERVICE_SETTINGS_SAVED ('AWS Integration: Service: Settings: Saved') includes an extra colon after 'Settings'. For consistency with similar events, consider removing the extra colon.
  • 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 points out an inconsistency, but looking at the full file, there isn't actually a clear standard - ACCOUNT_SETTINGS events don't use the colon, but SERVICE_SETTINGS events do. Without more context about the broader codebase's conventions, we can't be sure which pattern is correct. The comment is making assumptions about consistency without strong evidence.
    Maybe there's a good reason for the different colon patterns between ACCOUNT and SERVICE events? Maybe this helps distinguish different event categories?
    Even if the different patterns are intentional, the codebase should be consistent within each category. However, we don't have enough context to know which pattern is correct.
    Delete the comment. While consistency is good, we don't have strong evidence that this particular format is incorrect, and the comment makes assumptions without sufficient context.

Workflow ID: wflow_OkKw5eiNuaRzXzeK


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

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Incremental review on babf857 in 2 minutes and 7 seconds

More details
  • Looked at 235 lines of code in 8 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. frontend/src/container/CloudIntegrationPage/HeroSection/components/AccountSettingsModal.tsx:55
  • Draft comment:
    Typo in telemetry event: remove the extra apostrophe in the event name "AWS Integration: Account removed'".
  • Reason this comment was not posted:
    Marked as duplicate.
2. frontend/src/hooks/integration/aws/useIntegrationModal.ts:121
  • Draft comment:
    For consistency, consider using 'cloudAccountId' instead of 'id' in the telemetry payload (e.g. change to { cloudAccountId: data.account_id }).
  • Reason this comment was not posted:
    Comment was on unchanged code.
3. frontend/src/container/CloudIntegrationPage/HeroSection/components/AccountSettingsModal.tsx:55
  • Draft comment:
    Typo: The log message string includes an extraneous apostrophe at the end ("AWS Integration: Account removed'"). It should likely be corrected to remove the extra apostrophe.
  • Reason this comment was not posted:
    Marked as duplicate.
4. frontend/src/container/CloudIntegrationPage/HeroSection/components/RegionForm.tsx:59
  • Draft comment:
    Typographical issue: In the logEvent call on line 59, the event string is 'AWS Integration: Account connection attempt timed Out'. Consider changing it to 'AWS Integration: Account connection attempt timed out' 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 = 10% vs. threshold = 50%
    While this is technically correct and is about changed code, it's an extremely minor issue. The log event string is only used internally for telemetry and doesn't affect user experience or code functionality. The inconsistency would not cause any technical issues. The comment feels overly pedantic.
    The capitalization inconsistency could make it harder to search for specific log events if someone doesn't know the exact casing. It's also good practice to maintain consistency in logging strings.
    While consistency is good, this is too minor of an issue to warrant a PR comment. The log event will still be easily searchable and functional regardless of capitalization.
    Delete this comment as it's too minor and doesn't meaningfully impact code quality or functionality.
5. frontend/src/container/CloudIntegrationPage/ServicesSection/ConfigureServiceModal.tsx:139
  • Draft comment:
    There's an extra leading space in the value for rootClassName (line 139). Consider removing it for consistency (i.e., change " configure-service-modal" to "configure-service-modal").
  • 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_GuC62tZ94bIrEjiQ


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.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 c3e1544 in 48 seconds

More details
  • Looked at 26 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. frontend/src/container/CloudIntegrationPage/HeroSection/components/AccountSettingsModal.tsx:55
  • Draft comment:
    Fixed stray quote in logEvent string.
  • 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 states what was changed without offering any insight or guidance.
2. frontend/src/container/CloudIntegrationPage/HeroSection/components/RegionForm.tsx:59
  • Draft comment:
    Adjusted casing in logEvent text for consistency.
  • 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 states what was done without offering any insight or guidance.
3. frontend/src/container/CloudIntegrationPage/HeroSection/components/AccountSettingsModal.tsx:55
  • Draft comment:
    Fixed typo in logEvent message by removing the stray quote.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is purely informative, as it only states that a typo was fixed in a log message. It doesn't provide any actionable feedback or suggestions for improvement.
4. frontend/src/container/CloudIntegrationPage/HeroSection/components/RegionForm.tsx:59
  • Draft comment:
    Standardized logEvent message casing to 'timed out' instead of 'timed Out'.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is purely informative, as it just states what was changed without providing any actionable feedback or suggestions. It doesn't ask for confirmation or suggest improvements.
5. frontend/src/container/CloudIntegrationPage/HeroSection/components/AccountSettingsModal.tsx:74
  • Draft comment:
    There's a typographical error in the CSS class name 'account-settings-modal__body-regions-switch-switch' on line 74. It looks like the word 'switch' is repeated by mistake. Consider using a corrected class name like 'account-settings-modal__body-regions-switch' 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_h12JsRVtsMFHTtlx


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

Copy link
Collaborator

@ahmadshaheer ahmadshaheer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for making the changes 🙌👍🙏🙂

@raj-k-singh raj-k-singh enabled auto-merge (squash) February 26, 2025 17:35
@raj-k-singh raj-k-singh merged commit a3bc290 into main Feb 26, 2025
15 of 16 checks passed
@raj-k-singh raj-k-singh deleted the chore/aws-integration-ui-events branch February 26, 2025 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants