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

fix: Modifies messaging queue paylod #6783

Merged
merged 5 commits into from
Jan 9, 2025

Conversation

shivanshuraj1333
Copy link
Member

@shivanshuraj1333 shivanshuraj1333 commented Jan 8, 2025

related to https://github.com/SigNoz/engineering-pod/issues/2009
fixes: #6768


Important

This PR refines messaging queue payload handling by introducing Kafka-specific parsing and query logic, and sets up a framework for Celery integration.

  • Behavior:
    • Replaces ParseMessagingQueueBody with ParseKafkaQueueBody in http_handler.go for Kafka-specific parsing.
    • Introduces ParseQueueBody for generic queue parsing in http_handler.go.
    • Updates onboardProducers, onboardConsumers, and onboardKafka functions in http_handler.go to use Kafka-specific logic.
  • Kafka Integration:
    • Adds BuildOverviewQuery in queues/queueOverview.go for building overview queries.
    • Moves generateOverviewSQL from kafka/sql.go to queues/sql.go.
    • Refactors BuildClickHouseQuery and BuildQueryRangeParams in kafka/translator.go for improved query handling.
  • Celery Integration:
    • Adds translator.go in celery package as a placeholder for future Celery integration logic.
  • Misc:
    • Removes unused QueueFilters and CeleryTask structs from kafka/model.go.

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

Signed-off-by: Shivanshu Raj Shrivastava <[email protected]>
Signed-off-by: Shivanshu Raj Shrivastava <[email protected]>
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 ece8518 in 1 minute and 44 seconds

More details
  • Looked at 985 lines of code in 8 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. pkg/query-service/app/integrations/messagingQueues/celery/translator.go:1
  • Draft comment:
    The translator.go file in the celery package is currently empty. If this is intentional for future implementation, consider adding a comment to indicate that. Otherwise, ensure that any necessary code is added.
  • Reason this comment was not posted:
    Confidence changes required: 20%
    The file translator.go in the celery package is currently empty. This might be intentional if the implementation is planned for later, but it's worth noting in case it was accidentally left empty.
2. pkg/query-service/app/integrations/messagingQueues/queues/model.go:29
  • Draft comment:
    The QueueFilters struct is defined here and also in kafka/model.go. Consider consolidating these definitions to avoid duplication and potential maintenance issues.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The QueueFilters struct is defined in both kafka/model.go and queues/model.go. This duplication can lead to maintenance issues. Consider consolidating these definitions if they serve the same purpose.
3. pkg/query-service/app/integrations/messagingQueues/queues/sql.go:10
  • Draft comment:
    The generateOverviewSQL function is similar to a removed function in kafka/sql.go. Ensure that this new implementation covers all necessary cases and that the removal was intentional.
  • Reason this comment was not posted:
    Confidence changes required: 40%
    The generateOverviewSQL function in queues/sql.go is similar to a removed function in kafka/sql.go. Ensure that the new implementation covers all necessary cases and that the removal was intentional.
4. pkg/query-service/app/integrations/messagingQueues/celery/translator.go:1
  • 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 applicable in this file and other similar files in the PR.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_GTlAVbW0dtiCyJpc


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

Signed-off-by: Shivanshu Raj Shrivastava <[email protected]>
@shivanshuraj1333
Copy link
Member Author

As per discussion with @srikanthccv we want to have a unified json paylod across all new APIs, the earlier APIs were designed to have less payload and minimal configuration from frontend, but as per the discussion, it makes the APIs future proof to add enhancements in the product.

Hence doing some rework to support filters, #6757, and #6767 will be also be updated following similar logic.

The other new APIs would also use the similar payload.

@shivanshuraj1333 shivanshuraj1333 changed the title [fix] Modifies messaging queue paylod fix: Modifies messaging queue paylod Jan 8, 2025
Signed-off-by: Shivanshu Raj Shrivastava <[email protected]>
Signed-off-by: Shivanshu Raj Shrivastava <[email protected]>
@shivanshuraj1333 shivanshuraj1333 merged commit 2ead4fb into SigNoz:main Jan 9, 2025
15 of 16 checks passed
@shivanshuraj1333 shivanshuraj1333 deleted the feat/issues/6768 branch January 9, 2025 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Celery-Integration] Make consistent filtering across APIs
2 participants