feature: publisher confirms #169
Conversation
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
|
Warning Rate limit exceeded@rustatian has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 11 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in this pull request primarily involve significant updates to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
amqpjobs/driver.go(1 hunks)go.mod(2 hunks)tests/go.mod(2 hunks)
🔇 Additional comments (5)
go.mod (2)
17-20: LGTM! Consistent OpenTelemetry package updates.
The OpenTelemetry packages have been consistently updated from v1.31.0 to v1.32.0, maintaining version alignment across all related dependencies.
Also applies to: 37-37
17-22: Verify compatibility with the updated dependencies.
While these are minor version updates and should be backward compatible, it's important to ensure they don't introduce any breaking changes, especially given the focus on AMQP publisher confirms in this PR.
Also applies to: 37-37
✅ Verification successful
Let me check the specific usage of these dependencies to better understand their impact.
Let me check one more aspect - the specific OpenTelemetry API usage patterns to ensure there are no deprecated or changed APIs being used.
Dependencies update appears safe to proceed
The analysis shows standard and stable OpenTelemetry API usage patterns:
- Basic tracer provider and span operations
- Standard propagation APIs (TextMapPropagator, TraceContext, Baggage)
- Common global configuration patterns
- The sys package is only used for CPU feature detection in TLS configuration
The code uses well-established, stable APIs that are unlikely to have breaking changes in minor version updates. The AMQP publisher confirms functionality is independent of these dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any compatibility issues or breaking changes in the updated dependencies
# Test 1: Check if there are any direct uses of potentially changed OpenTelemetry APIs
echo "Checking OpenTelemetry API usage..."
rg -l "go.opentelemetry.io/(otel|contrib)" --type go
# Test 2: Look for any sys package usage that might be affected
echo "Checking sys package usage..."
rg -l "golang.org/x/sys" --type go
Length of output: 305
Script:
#!/bin/bash
# Check OpenTelemetry usage patterns
echo "=== OpenTelemetry Usage in plugin.go ==="
rg "go.opentelemetry.io/(otel|contrib)" plugin.go -B 2 -A 2
echo -e "\n=== OpenTelemetry Usage in amqpjobs/driver.go ==="
rg "go.opentelemetry.io/(otel|contrib)" amqpjobs/driver.go -B 2 -A 2
echo -e "\n=== OpenTelemetry Usage in amqpjobs/listener.go ==="
rg "go.opentelemetry.io/(otel|contrib)" amqpjobs/listener.go -B 2 -A 2
echo -e "\n=== sys Package Usage in amqpjobs/tls.go ==="
rg "golang.org/x/sys" amqpjobs/tls.go -B 2 -A 2
Length of output: 1336
Script:
#!/bin/bash
# Check specific OpenTelemetry API usage patterns
echo "=== Checking trace API usage ==="
rg "trace\." --type go -B 2 -A 2
echo -e "\n=== Checking propagation API usage ==="
rg "propagation\." --type go -B 2 -A 2
echo -e "\n=== Checking otel global API usage ==="
rg "otel\." --type go -B 2 -A 2
Length of output: 6558
tests/go.mod (3)
102-109: Standard library and Google API updates look appropriate.
The updates to golang.org/x/* packages and Google APIs are routine maintenance changes that bring in the latest improvements and security fixes. These changes are compatible with Go 1.23.2 as specified in the toolchain directive.
46-46: Core dependency updates require compatibility verification.
The updates to key dependencies:
- gRPC Gateway (v2.23.0)
- Temporal API (v1.41.0)
- gRPC (v1.68.0)
These updates are significant and may affect the RPC communication layer used by the AMQP publisher confirms feature.
#!/bin/bash
# Description: Check for potential breaking changes in RPC layer
# Test: Search for gRPC and Temporal API usage patterns that might need updates
rg "grpc\." -A 3
rg "temporal\." -A 3Also applies to: 98-98, 110-110
86-96: OpenTelemetry updates align with publisher confirms implementation.
The comprehensive update of OpenTelemetry components to v1.32.0 is beneficial for monitoring and observability of the new publisher confirms feature. This version includes improvements in tracing and metrics collection.
✅ Verification successful
Let me gather more information about OpenTelemetry usage in the codebase.
Let me check the actual OpenTelemetry implementation details to verify its usage.
OpenTelemetry dependency updates are properly integrated and utilized
The OpenTelemetry updates to v1.32.0 are well-justified as the codebase actively uses OpenTelemetry for tracing:
- Proper integration in
amqpjobs/driver.gowith trace propagation and SDK usage - Test environment configured with collector, Zipkin, and Jaeger support
- Active usage in the codebase through the otel plugin
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify OpenTelemetry usage in the codebase
# Test: Search for OpenTelemetry instrumentation to ensure it's properly utilized
rg -l "go.opentelemetry.io/otel"
Length of output: 2138
Script:
#!/bin/bash
# Search for OpenTelemetry imports and usage
rg -l "opentelemetry"
# Search specifically for otel instrumentation patterns
ast-grep --pattern 'otel.$_'
Length of output: 236
Script:
#!/bin/bash
# Check OpenTelemetry implementation details
rg -A 5 "opentelemetry" plugin.go
rg -A 5 "opentelemetry" amqpjobs/driver.go amqpjobs/listener.go
cat tests/env/docker-compose-amqp.yaml
Length of output: 2128
| // if error is not nil, ok would be false | ||
| if err != nil { | ||
| return errors.E(op, fmt.Errorf("failed to get publisher confirm: %w", err)) | ||
| } | ||
|
|
||
| // publish was unsuccessful | ||
| if !ok { | ||
| return errors.E(op, fmt.Errorf("failed to get publisher confirm")) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Correct the misleading comment and refine error handling
The comment on line 717 is misleading. If err is not nil, it indicates a communication error or context cancellation, not necessarily that ok would be false.
Additionally, when err is nil but ok is false, it means the broker negatively acknowledged the message (Nack). The error handling should reflect this distinction to provide clearer diagnostics.
Apply this diff to correct the comment and refine the error handling:
- // if error is not nil, ok would be false
- if err != nil {
- return errors.E(op, fmt.Errorf("failed to get publisher confirm: %w", err))
- }
-
- // publish was unsuccessful
- if !ok {
- return errors.E(op, fmt.Errorf("failed to get publisher confirm"))
- }
+ if err != nil {
+ // Communication error or context timeout
+ return errors.E(op, fmt.Errorf("failed to receive publisher confirm: %w", err))
+ }
+
+ if !ok {
+ // The broker negatively acknowledged the message
+ return errors.E(op, errors.Str("message was negatively acknowledged by the broker"))
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // if error is not nil, ok would be false | |
| if err != nil { | |
| return errors.E(op, fmt.Errorf("failed to get publisher confirm: %w", err)) | |
| } | |
| // publish was unsuccessful | |
| if !ok { | |
| return errors.E(op, fmt.Errorf("failed to get publisher confirm")) | |
| } | |
| if err != nil { | |
| // Communication error or context timeout | |
| return errors.E(op, fmt.Errorf("failed to receive publisher confirm: %w", err)) | |
| } | |
| if !ok { | |
| // The broker negatively acknowledged the message | |
| return errors.E(op, errors.Str("message was negatively acknowledged by the broker")) | |
| } |
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Reason for This PR
Description of Changes
basic.ackwas false.License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit
New Features
Bug Fixes
Chores