chore: active orders observability TODOs #469
Conversation
WalkthroughThe changes involve enhancements to error handling and logging in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant IngestUseCase as Ingest
participant OrderbookUseCaseImpl as Orderbook
participant TokensUseCase as Tokens
Client->>Ingest: Process Orderbook Pool
Ingest->>Ingest: Increment Error Counter if fails
Ingest->>Client: Return Status
Client->>Orderbook: GetActiveOrders()
Orderbook->>Orderbook: Log Error if fails
Orderbook->>Client: Return Active Orders
Client->>Orderbook: createLimitOrder()
Orderbook->>Tokens: GetSpotPriceScalingFactorByDenom()
Tokens-->>Orderbook: Return Scaling Factor
Orderbook->>Orderbook: Calculate Normalized Price
Orderbook->>Orderbook: Log Error if fails
Orderbook->>Client: Return Limit Order Status
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- ingest/usecase/ingest_usecase.go (1 hunks)
- orderbook/usecase/orderbook_usecase.go (5 hunks)
Additional comments not posted (4)
orderbook/usecase/orderbook_usecase.go (4)
134-138: LGTM! Consider implementing the alert mechanism.The elevated error logging enhances visibility. Consider implementing the suggested alert mechanism for better monitoring.
185-189: LGTM! Consider implementing the alert mechanism.The elevated error logging enhances visibility. Consider implementing the suggested alert mechanism for better monitoring.
232-235: LGTM!The updated normalization factor calculation and enhanced error handling improve the accuracy and robustness of the method.
302-302: LGTM! Verify the business logic.The updated calculation method for the normalized price may reflect a change in the intended business logic. Ensure this change aligns with the business requirements.
Please verify that the change in the calculation method aligns with the intended business logic.
| @@ -159,7 +163,7 @@ func (o *orderbookUseCaseImpl) GetActiveOrders(ctx context.Context, address stri | |||
| if !ok { | |||
| o.logger.Info("tick not found", zap.Any("contract", orderbook.ContractAddress), zap.Any("ticks", order.TickId), zap.Any("ok", ok)) | |||
|
|
|||
| // TODO: if tick not found, add an alert | |||
| // TODO: (alert) if tick not found, add an alert | |||
There was a problem hiding this comment.
Consider implementing the alert mechanism.
The TODO comment suggests adding an alert if a tick is not found. Implementing this will enhance monitoring.
Do you want me to generate the alert mechanism code or open a GitHub issue to track this task?
ingest/usecase/ingest_usecase.go
Outdated
| // TODO: (alert) if failed to process orderbook pool, add an alert | ||
| // Prometheus metric counter and alert | ||
|
|
There was a problem hiding this comment.
Consider implementing the alert mechanism.
The TODO comment suggests adding an alert if processing the orderbook pool fails. Implementing this will enhance monitoring.
Do you want me to generate the alert mechanism code or open a GitHub issue to track this task?
| @@ -0,0 +1,56 @@ | |||
| package telemetry | |||
There was a problem hiding this comment.
Would introducing Orderbook module level telemetry package help to lower conginitive load comparing of global one? I see following benefits:
- All gauges are scoped for specific module, easier navigate and to maintain
- Readability:
telemetry.GetTickByIDNotFoundCounter.Inc(), no need long names, for exampleSQSIngestHandlerProcessOrderbookPoolErrorCounter
cc @p0mvn
Introduce Prometheus error counters
deividaspetraitis
force-pushed
the
roman/add-observability-todos
branch
from
60da066 to
346d95f
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- domain/telemetry.go (3 hunks)
- ingest/usecase/ingest_usecase.go (1 hunks)
- orderbook/telemetry/telemetry.go (1 hunks)
- orderbook/usecase/orderbook_usecase.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- ingest/usecase/ingest_usecase.go
Additional comments not posted (13)
orderbook/telemetry/telemetry.go (7)
14-14: LGTM!The metric name is descriptive and follows a consistent naming convention.
The code changes are approved.
19-19: LGTM!The metric name is descriptive and follows a consistent naming convention.
The code changes are approved.
28-28: LGTM!The metric name is descriptive and follows a consistent naming convention.
The code changes are approved.
30-35: LGTM!The counter is correctly defined and initialized.
The code changes are approved.
37-42: LGTM!The counter is correctly defined and initialized.
The code changes are approved.
44-49: LGTM!The counter is correctly defined and initialized.
The code changes are approved.
52-55: LGTM!The init function is correctly registering the counters with Prometheus.
The code changes are approved.
domain/telemetry.go (3)
21-21: LGTM!The metric name is descriptive and follows a consistent naming convention.
The code changes are approved.
160-165: LGTM!The counter is correctly defined and initialized.
The code changes are approved.
298-298: LGTM!The init function is correctly registering the new counter with Prometheus.
The code changes are approved.
orderbook/usecase/orderbook_usecase.go (3)
135-136: LGTM!The telemetry counter for tracking errors during fetching active orders is correctly integrated. Elevating the logging level to
Errorenhances visibility.The code changes are approved.
162-163: LGTM!The telemetry counter for tracking the number of times a tick is not found by ID is correctly integrated.
The code changes are approved.
181-182: LGTM!The telemetry counter for tracking errors during creating limit orders is correctly integrated. Elevating the logging level to
Errorenhances visibility.The code changes are approved.
* fix: active orders overflow bug * chore: active orders observability TODOs * updates * lint * chore: active orders observability TODOs Introduce Prometheus error counters --------- Co-authored-by: Deividas Petraitis <[email protected]> (cherry picked from commit 4f5b8b8)
* fix: active orders overflow bug * chore: active orders observability TODOs * updates * lint * chore: active orders observability TODOs Introduce Prometheus error counters --------- Co-authored-by: Deividas Petraitis <[email protected]> (cherry picked from commit 4f5b8b8) Co-authored-by: Roman <[email protected]>
v25.17.0 tag contains: f1b7756 fix: no routes for this trade (#480) e6d44b6 perf: add load test for active orders (#472) bc570d6 active order fault tolerance via "best-effort" boolean return (#471) 37f4424 add concurrency to the order book processing in active orders (#470) 4f5b8b8 chore: active orders observability TODOs (#469) 7a28502 chore: add mergify backport on tag action (#473) b63ca05 fix: active orders overflow bug (#468)
* Update CHANGELOG.md v25.17.0 tag contains: f1b7756 fix: no routes for this trade (#480) e6d44b6 perf: add load test for active orders (#472) bc570d6 active order fault tolerance via "best-effort" boolean return (#471) 37f4424 add concurrency to the order book processing in active orders (#470) 4f5b8b8 chore: active orders observability TODOs (#469) 7a28502 chore: add mergify backport on tag action (#473) b63ca05 fix: active orders overflow bug (#468) * Update CHANGELOG.md Co-authored-by: Roman <[email protected]> * Update CHANGELOG.md Co-authored-by: Roman <[email protected]> * Update CHANGELOG.md Co-authored-by: Roman <[email protected]> --------- Co-authored-by: Roman <[email protected]>
* Update CHANGELOG.md v25.17.0 tag contains: f1b7756 fix: no routes for this trade (#480) e6d44b6 perf: add load test for active orders (#472) bc570d6 active order fault tolerance via "best-effort" boolean return (#471) 37f4424 add concurrency to the order book processing in active orders (#470) 4f5b8b8 chore: active orders observability TODOs (#469) 7a28502 chore: add mergify backport on tag action (#473) b63ca05 fix: active orders overflow bug (#468) * Update CHANGELOG.md Co-authored-by: Roman <[email protected]> * Update CHANGELOG.md Co-authored-by: Roman <[email protected]> * Update CHANGELOG.md Co-authored-by: Roman <[email protected]> --------- Co-authored-by: Roman <[email protected]> (cherry picked from commit 1f6db39)
* Update CHANGELOG.md v25.17.0 tag contains: f1b7756 fix: no routes for this trade (#480) e6d44b6 perf: add load test for active orders (#472) bc570d6 active order fault tolerance via "best-effort" boolean return (#471) 37f4424 add concurrency to the order book processing in active orders (#470) 4f5b8b8 chore: active orders observability TODOs (#469) 7a28502 chore: add mergify backport on tag action (#473) b63ca05 fix: active orders overflow bug (#468) * Update CHANGELOG.md Co-authored-by: Roman <[email protected]> * Update CHANGELOG.md Co-authored-by: Roman <[email protected]> * Update CHANGELOG.md Co-authored-by: Roman <[email protected]> --------- Co-authored-by: Roman <[email protected]> (cherry picked from commit 1f6db39) Co-authored-by: Calvin <[email protected]>
We need to add these. Making this PR for tracking. Please
Summary by CodeRabbit
New Features
Bug Fixes
Chores