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

feat: added table column and row logic for the new api response structure for producer overview #6433

Merged
merged 4 commits into from
Dec 13, 2024

Conversation

SagarRajput-7
Copy link
Contributor

@SagarRajput-7 SagarRajput-7 commented Nov 12, 2024

Summary

We have a new API response structure, due to which need to change the column and table data handling -

slack thread for reference - https://signoz-team.slack.com/archives/C070V9S041L/p1731413223737439

Related Issues / PR's

Screenshots

Screen.Recording.2024-12-10.at.10.47.27.AM.mov

Affected Areas and Manually Tested Areas

  • Tested producer overview table under Producer Latency View

Important

Add logic for handling new API response structure for producer latency overview in messaging queues.

  • New Functionality:
    • Add getColumnsForProducerLatencyOverview and getTableDataForProducerLatencyOverview in MQTableUtils.tsx for new API response structure.
    • Introduce TopicThroughputProducerOverviewResponse interface in MQTableUtils.tsx.
  • Integration:
    • Update MessagingQueuesTable in MQTables.tsx to use new utility functions for producer latency overview.
    • Modify getTopicThroughputOverview in getTopicThroughputOverview.ts to return TopicThroughputProducerOverviewResponse.
  • Misc:
    • Remove evalTime from MessagingQueueOverview.tsx.
    • Refactor MQDetails.tsx to remove redundant code and update imports.

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

Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

@github-actions github-actions bot added docs required enhancement New feature or request labels Nov 12, 2024
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

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. Reviewed everything up to 87fe6cd in 1 minute and 16 seconds

More details
  • Looked at 209 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. frontend/src/pages/MessagingQueues/MQDetails/MQTables/MQTables.tsx:36
  • Draft comment:
    Typo in function name getColumnsForProduderLatencyOverview. It should be getColumnsForProducerLatencyOverview. This typo is also present in other files where this function is used.
  • Reason this comment was not posted:
    Marked as duplicate.
2. frontend/src/pages/MessagingQueues/MQDetails/MQTables/MQTables.tsx:197
  • Draft comment:
    Typo in function name getColumnsForProduderLatencyOverview. It should be getColumnsForProducerLatencyOverview. This typo is also present in other files where this function is used.
  • Reason this comment was not posted:
    Marked as duplicate.
3. frontend/src/pages/MessagingQueues/MQDetails/MQTables/getTopicThroughputOverview.ts:8
  • Draft comment:
    Typo in function name getColumnsForProduderLatencyOverview. It should be getColumnsForProducerLatencyOverview. This typo is also present in other files where this function is used.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_vdZRYxKuNkKIW39E


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

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

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 531becc in 29 seconds

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

Workflow ID: wflow_zPS8AVigd4SULVQJ


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

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

@vikrantgupta25 vikrantgupta25 changed the title feat: added table column and row logic for the new api response structure for prodcure overview feat: added table column and row logic for the new api response structure for producer overview Nov 15, 2024
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

Copy link
Collaborator

@vikrantgupta25 vikrantgupta25 left a comment

Choose a reason for hiding this comment

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

i remember we had a discussion to move these to api folder instead of pages ?

@SagarRajput-7 SagarRajput-7 force-pushed the producer-overview-kafka-api-fix branch from 27c855e to ff7f1a8 Compare December 9, 2024 14:54
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 ff7f1a8 in 13 seconds

More details
  • Looked at 36 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. frontend/src/pages/MessagingQueues/MQDetails/MQTables/MQTableUtils.tsx:27
  • Draft comment:
    Typo in function name corrected from getColumnsForProduderLatencyOverview to getColumnsForProducerLatencyOverview. Ensure all references are updated accordingly.
  • Reason this comment was not posted:
    Confidence changes required: 10%
    The function name is misspelled in the original code and has been corrected in the PR. This is a straightforward fix.

Workflow ID: wflow_LbLmJo7hPUiGwWbg


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

Copy link

github-actions bot commented Dec 9, 2024

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

1 similar comment
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

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 fdd0c1d in 53 seconds

More details
  • Looked at 427 lines of code in 5 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. frontend/src/api/messagingQueues/getTopicThroughputOverview.ts:14
  • Draft comment:
    Consider adding error handling for the axios request to handle potential request failures gracefully.
  • Reason this comment was not posted:
    Comment was on unchanged code.
2. frontend/src/pages/MessagingQueues/MQDetails/MQTables/MQTableUtils.tsx:12
  • Draft comment:
    Ensure that data.result has at least two elements before accessing data.result[1] to avoid potential runtime errors.
  • Reason this comment was not posted:
    Comment did not seem useful.
3. frontend/src/pages/MessagingQueues/MQDetails/MQTables/MQTables.tsx:49
  • Draft comment:
    Ensure that data.result[0]?.table?.columns is defined before attempting to spread it into mergedColumns to prevent potential runtime errors.
  • Reason this comment was not posted:
    Marked as duplicate.
4. frontend/src/pages/MessagingQueues/MQDetails/MQTables/MQTables.tsx:52
  • Draft comment:
    Ensure that data.result[0]?.table?.columns is defined before attempting to spread it into mergedColumns to prevent potential runtime errors.
  • Reason this comment was not posted:
    Marked as duplicate.
5. frontend/src/pages/MessagingQueues/MQDetails/MQTables/MQTables.tsx:32
  • Draft comment:
    Avoid using hardcoded color values in stylesheets. Use design tokens or predefined color constants instead. This applies to other stylesheets as well.
  • Reason this comment was not posted:
    Comment was not on a valid diff hunk.

Workflow ID: wflow_SYaqTxhvCgO1SYdx


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

Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

@SagarRajput-7 SagarRajput-7 force-pushed the producer-overview-kafka-api-fix branch from fdd0c1d to bcc55f4 Compare December 10, 2024 05:38
@SagarRajput-7
Copy link
Contributor Author

i remember we had a discussion to move these to api folder instead of pages ?

@vikrantgupta25 - this is been done under this PR now - #6611

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.

I noticed a minor formatting issue: the decimal places in the floating point numbers are currently showing full precision, while the details table uses 3 decimal places. Can you limit these to 3 decimal places for consistency?

image

Also added a minor comment inline. Please feel free to ignore/take care of it in another PR.

just needs the decimal formatting adjustment, rest LGTM.

vikrantgupta25
vikrantgupta25 previously approved these changes Dec 12, 2024
@SagarRajput-7
Copy link
Contributor Author

I noticed a minor formatting issue: the decimal places in the floating point numbers are currently showing full precision, while the details table uses 3 decimal places. Can you limit these to 3 decimal places for consistency?

image

Also added a minor comment inline. Please feel free to ignore/take care of it in another PR.

just needs the decimal formatting adjustment, rest LGTM.

Done, Thanks!

Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

@SagarRajput-7 SagarRajput-7 force-pushed the producer-overview-kafka-api-fix branch from cd2e17f to 3bfd93d Compare December 12, 2024 04:01
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 cd2e17f in 36 seconds

More details
  • Looked at 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. frontend/src/pages/MessagingQueues/MQDetails/MQTables/MQTables.tsx:74
  • Draft comment:
    Consider using a more reliable utility for checking if a value is a number, such as Number.isFinite or a custom utility function, instead of isNumber from chart.js/helpers. This comment also applies to other instances where isNumber is used.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code uses isNumber from chart.js/helpers, which is not a standard utility for checking numbers. It might be better to use a more reliable utility or a custom function.
2. frontend/src/pages/MessagingQueues/MQDetails/MQTables/MQTables.tsx:67
  • Draft comment:
    Avoid using the component/index.tsx file structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. This is also applicable to other similar files in the project.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_Wx47CTvBaBtjUt32


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

@SagarRajput-7 SagarRajput-7 merged commit 8c46de8 into develop Dec 13, 2024
17 checks passed
@SagarRajput-7 SagarRajput-7 deleted the producer-overview-kafka-api-fix branch December 13, 2024 05:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs not required enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants