-
Notifications
You must be signed in to change notification settings - Fork 130
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: kq commitinorder support trace #75
base: master
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.
PR Summary
Added tracing support to the CommitInOrder functionality in the Kafka queue implementation, enhancing message processing traceability.
- Introduced
extractCtxFromMsg
function to consistently extract context from Kafka messages - Applied
extractCtxFromMsg
in both CommitInOrder and regular consumer cases - Improved traceability of message processing in the queue system
- Ensured consistent context extraction across different consumption modes
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
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.
1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
if err := q.consumeOne(context.Background(), string(msg.Key), string(msg.Value)); err != nil { | ||
ctx := extractCtxFromMsg(msg) | ||
|
||
if err := q.consumeOne(ctx, string(msg.Key), string(msg.Value)); err != nil { | ||
if q.errorHandler != nil { | ||
q.errorHandler(context.Background(), msg, err) |
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.
logic: using context.Background() here loses trace context - should use ctx instead
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.
LGTM
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
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.
1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
defer func() { | ||
if err := recover(); err != nil { | ||
logc.Errorf(ctx, "consumeOne failed recover, error: %v", err) | ||
} | ||
}() |
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.
style: recover handler should re-panic on system errors like out of memory that shouldn't be caught
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.
1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
go.mod
Outdated
@@ -1,4 +1,4 @@ | |||
module github.com/zeromicro/go-queue | |||
module github.com/zhuud/go-queue |
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.
logic: Module path should remain 'github.com/zeromicro/go-queue' as this is a PR to the original repository. This change would break all existing imports.
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.
LGTM
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
Greptile Summary
Added OpenTelemetry tracing support to Kafka queue implementation, enabling distributed tracing across message processing with standardized context propagation.
kq/internal/trace.go
implementingMessageCarrier
interface for OpenTelemetry context handlingPushWithKey
method using OpenTelemetry propagatorextractCtxFromMsg
function for consistent tracingconsumeOne
methodgo.mod
to support OpenTelemetry integration💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!