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.

Sign up for GitHub

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

Jump to bottom

[feat] Add overview APIs for Celery and Kafka in messaging queues integration #6756

Merged
merged 5 commits into from
Jan 8, 2025
Merged

[feat] Add overview APIs for Celery and Kafka in messaging queues integration #6756

merged 5 commits into from
Jan 8, 2025
+327 −38

Conversation

shivanshuraj1333
Copy link
Member

@shivanshuraj1333 shivanshuraj1333 commented Jan 5, 2025 •
edited
Loading

Related to: https://github.com/SigNoz/engineering-pod/issues/2009

Fixes: #6755

Important

Add new API routes and SQL query generation for Celery and Kafka messaging queues, with corresponding tests.

  • API Routes:
    • Added /api/v1/messaging-queues/queue-overview for general queue overview.
    • Added /api/v1/messaging-queues/celery routes for Celery overview, tasks, and performance.
    • Refactored Kafka routes under /api/v1/messaging-queues/kafka.
  • Query Generation:
    • Added generateOverviewSQL() in sql.go for queue overview queries.
    • Added generateProducerConsumerEvalSQL() in sql.go for producer-consumer evaluation.
  • Models:
    • Added QueueFilters and CeleryTask structs in model.go.
  • Tests:
    • Added tests for new SQL functions in sql_test.go.
    • Added tests for new models in model_test.go.
    • Added tests for query parameter building in translator_test.go.

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

ellipsis-dev[bot]
ellipsis-dev bot reviewed Jan 5, 2025
View reviewed changes
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 a4cb482 in 2 minutes and 43 seconds

More details
  • Looked at 1258 lines of code in 7 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. pkg/query-service/app/integrations/messagingQueues/kafka/model_test.go:160
  • Draft comment:
    The wantKind should be "worker" instead of "active" to match the expected behavior of GetKind.
			wantKind: "worker",
  • Reason this comment was not posted:
    Marked as duplicate.
2. pkg/query-service/app/integrations/messagingQueues/kafka/model_test.go:183
  • Draft comment:
    The GetKind method should return "worker" instead of "active" to match the expected behavior.
		assert.Equal(t, "worker", task.GetKind(), "GetKind should return kind")
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    Looking at the test cases and implementation: 1) The struct initializes kind="worker" and status="active" 2) The test expects GetKind() to return "active" 3) This seems inconsistent with field naming, as GetKind() should logically return the 'kind' field 4) However, without seeing the actual CeleryTask implementation, I can't be 100% certain this is wrong 5) The test is consistent with itself across both TestCeleryTask and TestCeleryTaskInterface.
    Without seeing the actual implementation of CeleryTask and its GetKind() method, I can't be completely certain that returning status instead of kind is incorrect. The consistent test behavior suggests this might be intentional.
    While the field naming suggests GetKind() should return the kind field, the consistent test behavior and lack of implementation visibility means we can't be confident enough to declare this as definitely wrong.
    Without seeing the CeleryTask implementation, we can't be confident enough that this is actually a bug rather than intentional behavior.
3. pkg/query-service/app/integrations/messagingQueues/kafka/model.go:48
  • Draft comment:
	return r.kind
  • Reason this comment was not posted:
    Marked as duplicate.
4. pkg/query-service/app/integrations/messagingQueues/kafka/model.go:57
  • Draft comment:
	r.status = status
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_wONOujMOIb5z66UF

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/model.go Outdated Show resolved Hide resolved
pkg/query-service/app/integrations/messagingQueues/kafka/model.go Outdated Show resolved Hide resolved
@shivanshuraj1333 shivanshuraj1333 force-pushed the feat/issues/6755 branch 2 times, most recently from 60cad1a to 6f9271f Compare January 5, 2025 23:53
@shivanshuraj1333
Copy link
Member Author

shivanshuraj1333 commented Jan 6, 2025 •
edited
Loading

@srikanthccv I want to open up some more PRs, but those are making some slight changes on top of this, it would be a good idea to either get it merged, or I add code changes to this one (and make a big PR).

let me know WDYT

srikanthccv
srikanthccv reviewed Jan 7, 2025
View reviewed changes
Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

I think it's a good idea to keep the base changes setup and their reasoning in this PR. In the next PRs, then I don't have to think about them again

@shivanshuraj1333
feat: added queue overview api
28f19d8 
Signed-off-by: Shivanshu Raj Shrivastava <[email protected]>
@shivanshuraj1333
feat: add filtering
8f238f3 
Signed-off-by: Shivanshu Raj Shrivastava <[email protected]>
@shivanshuraj1333
chore: update messaging system name
807c1c6 
Signed-off-by: Shivanshu Raj Shrivastava <[email protected]>
@shivanshuraj1333
chore: update query
50ac256 
Signed-off-by: Shivanshu Raj Shrivastava <[email protected]>
@shivanshuraj1333
Copy link
Member Author

@srikanthccv addressed the other changes, apart from filter is there anything else?
Would want to keep this PR going, filtering refactoring I need to do across all the branches (and open PRs).
So, it's better I do it in next iteration, and make the code APIs ready for UI.

The filtering refactoring will be done via this post first merge #6768

srikanthccv
srikanthccv approved these changes Jan 8, 2025
View reviewed changes
Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

All the custom parsing and query building means there could be number of issue with how they are done. I am not reviewing them since you want to address them later

@shivanshuraj1333 shivanshuraj1333 removed the request for review from YounixM January 8, 2025 06:47
@shivanshuraj1333 shivanshuraj1333 enabled auto-merge (squash) January 8, 2025 06:48
@shivanshuraj1333 shivanshuraj1333 merged commit 505757b into SigNoz:main Jan 8, 2025
15 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ellipsis-dev ellipsis-dev[bot] ellipsis-dev[bot] left review comments

@ankitnayan ankitnayan ankitnayan left review comments

@srikanthccv srikanthccv srikanthccv approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@shivanshuraj1333 @ankitnayan @srikanthccv