[feat] Add celery-overview APIs #6757
Draft
+198
−17
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 4f9a90e in 1 minute and 43 seconds
More details
- Looked at
646lines of code in4files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. pkg/query-service/app/integrations/messagingQueues/kafka/sql.go:321
- Draft comment:
The functiongenerateOverviewSQLdoes not use thequeueTypeparameter, which might be necessary for future extensions or clarity. Consider including it in the function signature and using it appropriately if needed. - Reason this comment was not posted:
Confidence changes required:50%
The functiongenerateOverviewSQLinsql.gois not using thequeueTypeparameter, which might be necessary for future extensions or clarity. It should be included in the function signature and used appropriately if needed.
2. pkg/query-service/app/integrations/messagingQueues/kafka/model.go:28
- Draft comment:
TheQueueFiltersstruct has fields that are not exported, which might cause issues if they need to be accessed outside the package. Consider exporting these fields if they are intended to be used outside the package. - Reason this comment was not posted:
Confidence changes required:50%
TheQueueFiltersstruct inmodel.gohas fields that are not exported, which might cause issues if they need to be accessed outside the package. Consider exporting these fields if they are intended to be used outside the package.
3. pkg/query-service/app/integrations/messagingQueues/kafka/model.go:36
- Draft comment:
TheCeleryTaskstruct fields are not exported, which might cause issues if they need to be accessed outside the package. Consider exporting these fields if they are intended to be used outside the package. - Reason this comment was not posted:
Confidence changes required:50%
TheCeleryTaskstruct fields are not exported, which might cause issues if they need to be accessed outside the package. Consider exporting these fields if they are intended to be used outside the package.
Workflow ID: wflow_5lXshYAteuXNnEaK
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.
pkg/query-service/app/integrations/messagingQueues/kafka/translator.go
Outdated
Show resolved
Hide resolved
shivanshuraj1333
force-pushed
the
feat/issues/6754
branch
from
26ccd12 to
2969271
Compare
|
This PR should be kept in draft state as it does not implement celery specific things yet like |
shivanshuraj1333
force-pushed
the
feat/issues/6754
branch
from
2969271 to
53ba68a
Compare
Signed-off-by: Shivanshu Raj Shrivastava <[email protected]>
Signed-off-by: Shivanshu Raj Shrivastava <[email protected]>
Signed-off-by: Shivanshu Raj Shrivastava <[email protected]>
Signed-off-by: Shivanshu Raj Shrivastava <[email protected]>
Signed-off-by: Shivanshu Raj Shrivastava <[email protected]>
Signed-off-by: Shivanshu Raj Shrivastava <[email protected]>
shivanshuraj1333
force-pushed
the
feat/issues/6754
branch
from
53ba68a to
5213297
Compare
|
This will be updated with #6768 to support generic filtering later on using QB based filters |
|
@srikanthccv this is ready for review |
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 5213297 in 2 minutes and 36 seconds
More details
- Looked at
300lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. pkg/query-service/app/integrations/messagingQueues/kafka/translator.go:408
- Draft comment:
TheStatusfield is checked inGetCeleryFiltersbut not used ingetCeleryOverview. EnsureStatusis necessary or remove the check to avoid confusion. - Reason this comment was not posted:
Comment did not seem useful.
2. pkg/query-service/app/integrations/messagingQueues/kafka/translator.go:143
- Draft comment:
Ensurefilters.QueryForis not empty before accessingfilters.QueryFor[0]to prevent potential runtime panics. - Reason this comment was not posted:
Comment did not seem useful.
3. pkg/query-service/app/integrations/messagingQueues/kafka/translator.go:128
- Draft comment:
Avoid adding non-ClickHouse related functions likeCeleryClickHouseQueryto the ClickHouseReader interface. Consider moving it to a Celery-specific module or file. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment seems to misunderstand the purpose of the function. CeleryClickHouseQuery is building ClickHouse queries for Celery metrics, similar to how other functions in this file build ClickHouse queries for Kafka metrics. The function belongs in this file as it's part of the query translation layer. The comment's premise that this isn't ClickHouse related is incorrect.
Could there still be value in separating Celery-specific code from Kafka-specific code for better organization, even if both use ClickHouse?
While separating concerns is generally good, this file appears to be the translation layer for messaging queue metrics to ClickHouse queries. Both Kafka and Celery are messaging systems, so having their query builders in the same place is reasonable.
The comment should be deleted as it incorrectly assumes the function isn't ClickHouse related, when it actually is building ClickHouse queries just like other functions in this file.
Workflow ID: wflow_QD98wVTFJcGNVwQI
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.
Signed-off-by: Shivanshu Raj Shrivastava <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related to: https://github.com/SigNoz/engineering-pod/issues/2009
Fixes: #6754
Important
Add Celery-specific API endpoints and backend logic for overview, tasks, and performance in messaging queues.
/api/v1/messaging-queues/celery/overview,/tasks, and/performanceroutes inhttp_handler.go.getCeleryOverview()inhttp_handler.goto handle Celery overview requests.CeleryClickHouseQuery()intranslator.gofor building query parameters for Celery.generateOverviewSQL()insql.goto support overview queries for both Kafka and Celery.CeleryTaskandCeleryTasksinterface inmodel.gofor task representation.BuildClickHouseQuery()intranslator.goto include Celery context.This description was created by
for 5213297. It will automatically update as commits are pushed.