-
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] Add celery-overview APIs #6757
base: main
Are you sure you want to change the base?
Conversation
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 4f9a90e in 1 minute and 43 seconds
More details
- Looked at
646
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. pkg/query-service/app/integrations/messagingQueues/kafka/sql.go:321
- Draft comment:
The functiongenerateOverviewSQL
does not use thequeueType
parameter, 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 functiongenerateOverviewSQL
insql.go
is not using thequeueType
parameter, 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:
TheQueueFilters
struct 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%
TheQueueFilters
struct inmodel.go
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.
3. pkg/query-service/app/integrations/messagingQueues/kafka/model.go:36
- Draft comment:
TheCeleryTask
struct 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%
TheCeleryTask
struct 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
26ccd12
to
2969271
Compare
This PR should be kept in draft state as it does not implement celery specific things yet like |
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]>
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.
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 5213297 in 2 minutes and 36 seconds
More details
- Looked at
300
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. pkg/query-service/app/integrations/messagingQueues/kafka/translator.go:408
- Draft comment:
TheStatus
field is checked inGetCeleryFilters
but not used ingetCeleryOverview
. EnsureStatus
is 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.QueryFor
is 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 likeCeleryClickHouseQuery
to 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]>
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/performance
routes inhttp_handler.go
.getCeleryOverview()
inhttp_handler.go
to handle Celery overview requests.CeleryClickHouseQuery()
intranslator.go
for building query parameters for Celery.generateOverviewSQL()
insql.go
to support overview queries for both Kafka and Celery.CeleryTask
andCeleryTasks
interface inmodel.go
for task representation.BuildClickHouseQuery()
intranslator.go
to include Celery context.This description was created by for 5213297. It will automatically update as commits are pushed.