-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 byte rate to producer API #6579
base: develop
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 d016e49 in 1 minute and 48 seconds
More details
- Looked at
164
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. pkg/query-service/app/http_handler.go:3202
- Draft comment:
The variable namelatencySeries
is misleading as it now deals with byte rate. Consider renaming it tobyteRateSeries
for clarity. - Reason this comment was not posted:
Marked as duplicate.
2. pkg/query-service/app/http_handler.go:3221
- Draft comment:
The functionpostprocess.TransformToTableForBuilderQueries
is used for transforming the resultFetchLatency. Ensure that this function is appropriate for handling byte rate data as well. - Reason this comment was not posted:
Confidence changes required:50%
The PR changes the query context from 'producer-throughput-overview-latency' to 'producer-throughput-overview-byte-rate'. However, the function name and variable names still refer to latency, which can be misleading. This should be updated to reflect the new context of byte rate.
3. pkg/query-service/app/integrations/messagingQueues/kafka/translator.go:283
- Draft comment:
The functionbuildBuilderQueriesProducerBytes
is used for building queries related to byte rate. Ensure that this function is appropriate for handling byte rate data as well. - Reason this comment was not posted:
Confidence changes required:50%
The PR changes the query context from 'producer-throughput-overview-latency' to 'producer-throughput-overview-byte-rate'. However, the function name and variable names still refer to latency, which can be misleading. This should be updated to reflect the new context of byte rate.
4. pkg/query-service/app/integrations/messagingQueues/kafka/translator.go:279
- Draft comment:
Avoid using thecomponent/index.tsx
file structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_BwmxoRJ4eh9MoGAi
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.
I see there is still a list panel type for one specific query https://github.com/shivanshuraj1333/signoz/blob/5e44803e47bbf95e54644e183a602a10b14da97b/pkg/query-service/app/integrations/messagingQueues/kafka/translator.go#L375. trying to understand, why do keep some for the table and some list? |
the code that you highlighted is not used in the For the highlighted one, it makes sense to use list, and it's powering another API |
fixes: https://github.com/SigNoz/engineering-pod/issues/2056
Important
Add byte rate metric to producer throughput overview in the producer API, updating query parameters and functions accordingly.
getProducerThroughputOverview()
inhttp_handler.go
.producer-throughput-overview-byte-rate
inBuildQRParamsWithCache()
intranslator.go
.queryRangeParams
toproducerQueryRangeParams
ingetProducerThroughputOverview()
inhttp_handler.go
.buildBuilderQueriesProducerBytes()
to usebyte_rate
as query name intranslator.go
.latencyColumn
tobyte_rate
ingetProducerThroughputOverview()
inhttp_handler.go
.PanelType
toTable
inBuildQRParamsWithCache()
intranslator.go
.This description was created by for d016e49. It will automatically update as commits are pushed.