[feat] Adds celery performance APIs #6767
Draft
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 8078cbb in 2 minutes and 41 seconds
More details
- Looked at
926lines of code in4files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. pkg/query-service/app/http_handler.go:4973
- Draft comment:
Inconsistent error handling: useerrinstead ofapiErrin the second error handling block to ensure correct error reporting. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The code shows error handling for a messaging queue API endpoint. The first error check uses apiErr from ParseMessagingQueueBody(), while the second uses err from BuildClickHouseQuery(). These are actually different errors from different functions - apiErr is a parsed API error while err is a regular error. Using different variables makes sense here since they come from different sources. The comment's suggestion to use err consistently would actually be incorrect.
Could I be missing some subtle error handling best practice? Maybe there's a reason to standardize on one error type?
No - in this case the different error types are intentional and appropriate. apiErr is specifically for API-level errors while err is for internal errors. Converting between them would lose error type information.
The comment should be deleted. The error handling is correct as-is, using appropriate error types for different error sources.
2. pkg/query-service/app/integrations/messagingQueues/kafka/sql.go:333
- Draft comment:
Consider parameterizingtsBucketStartandtsBucketEndor documenting the logic behind these hardcoded values to ensure future maintainability and correctness. - Reason this comment was not posted:
Confidence changes required:50%
ThegenerateOverviewSQLfunction uses hardcoded values fortsBucketStartandtsBucketEnd. This could lead to incorrect query results if the logic for these values changes. Consider parameterizing these values or documenting the logic behind them.
3. pkg/query-service/app/integrations/messagingQueues/kafka/model.go:48
- Draft comment:
Consider exporting theCeleryTaskstruct fields if they need to be accessed outside the package. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The code follows a proper encapsulation pattern by keeping fields private and providing access through an interface with getter methods. This is a common Go pattern for information hiding. Making the fields public would bypass the interface abstraction. The current design is intentional and correct.
Maybe there are use cases where direct field access would be more efficient than going through getters. The interface might be unnecessary overhead.
The interface pattern provides better encapsulation and flexibility for future changes. The performance difference would be negligible, and the architectural benefit outweighs any minor performance gain.
The comment should be deleted as it suggests breaking a well-designed encapsulation pattern. The current implementation with private fields and an interface is the correct approach.
4. pkg/query-service/app/integrations/messagingQueues/kafka/translator.go:64
- Draft comment:
TheattributeCacheparameter is not used in thebuildBuilderQueriesProducerBytesfunction. Review if this parameter is necessary or if its data should be utilized. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_SY7mkAxQhsNijOiC
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]>
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/6752
branch
from
8078cbb to
c960d0f
Compare
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to c960d0f in 2 minutes and 47 seconds
More details
- Looked at
440lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. pkg/query-service/app/http_handler.go:5002
- Draft comment:
ReusingapiErrfor different error types can lead to incorrect error responses. Consider using separate error variables for different error types. This issue is also present ingetCeleryPerformance. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment raises a theoretical concern about error handling, but in practice:
- The apiErr variable is only used to pass errors to RespondError()
- Each error case returns immediately after calling RespondError()
- The error handling pattern is consistent throughout the codebase
- No actual bugs or issues would arise from this pattern
Could there be subtle error handling bugs I'm missing? The comment author may have spotted a real issue with error propagation that isn't immediately obvious.
After careful review, the error handling is safe because each error case returns immediately after handling and the error variable is not used for control flow or logic.
The comment should be deleted. The current error handling pattern is safe and standard Go practice. Changing it would add unnecessary complexity without any real benefit.
2. pkg/query-service/app/http_handler.go:5056
- Draft comment:
ThegetCeleryTasksfunction is not implemented. Implement this function to handle Celery tasks logic as per the PR description. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is pointing out something that is clearly visible in the code - an empty function. The PR appears to be adding Celery functionality incrementally, with some functions implemented and others left as TODOs. The empty function with a TODO comment is a common and valid pattern for incremental development. The comment doesn't add any value beyond what's already obvious from the code.
The comment could be valid if the PR description specifically stated that this function should be implemented in this PR. However, without seeing the PR description, I can't be certain.
Even if the PR description mentioned implementing this function, the comment doesn't provide any specific guidance or requirements. It's just pointing out what's already obvious from the code. The TODO comment in the code already indicates future implementation is needed.
The comment should be deleted as it doesn't provide any actionable feedback beyond what's already obvious from the code itself.
3. pkg/query-service/app/integrations/messagingQueues/kafka/translator.go:143
- Draft comment:
Check iffilters.QueryForis non-empty before accessingfilters.QueryFor[0]to avoid potential runtime panic. This issue is also present inbuildCeleryPerformanceQuery. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The code already has validation in GetCeleryFilters() that ensures filters.QueryFor has exactly 1 element before it's used. The validation happens before any code that accesses filters.QueryFor[0]. Since the validation is already in place, this comment is suggesting redundant checks.
Could there be cases where GetCeleryFilters() is bypassed and the filters are used directly? Could there be future code changes that remove the validation?
Looking at the code flow, CeleryClickHouseQuery is only called after GetCeleryFilters() validation. The validation is a core part of the API contract and unlikely to be removed.
The comment should be deleted since the code already has proper validation in GetCeleryFilters() that prevents the potential panic scenario.
Workflow ID: wflow_hOh0YLsWx1uwyIbf
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.
| @@ -321,14 +489,46 @@ func BuildQRParamsWithCache( | |||
| return queryRangeParams, err | |||
| } | |||
|
|
|||
| func GetCeleryFilters(variables map[string]string) (*QueueFilters, error) { | |||
| filters := getFilters(variables) | |||
| if len(filters.QueryFor) != 1 || len(filters.Status) != 1 { | |||
There was a problem hiding this comment.
Consider adding a check for filters.LatencyType length if it is expected to have a specific length, similar to filters.QueryFor and filters.Status.
| @@ -126,6 +125,175 @@ | |||
| return bq, nil | |||
| } | |||
|
|
|||
| func CeleryClickHouseQuery( | |||
There was a problem hiding this comment.
Avoid adding Celery-specific query functions like CeleryClickHouseQuery to the ClickHouseReader interface. Use the DAO in the telemetry instance instead.
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.
Fixes: #6752
Related to https://github.com/SigNoz/engineering-pod/issues/2009
Important
Adds Celery performance APIs with new routes, handlers, and SQL query support for Celery data in the messaging queue system.
RegisterMessagingQueuesRoutes()inhttp_handler.gofor/overview,/tasks, and/performance.getCeleryOverview(),getCeleryTasks(), andgetCeleryPerformance()inhttp_handler.go.generateOverviewSQL()andgenerateProducerConsumerEvalSQL()insql.gofor Celery data.CeleryClickHouseQuery()andbuildCeleryPerformanceQuery()intranslator.gofor constructing queries for Celery metrics.WorkerResponseandQueueFiltersstructs inmodel.gofor handling Celery data.This description was created by
for c960d0f. It will automatically update as commits are pushed.