Chore: Adds detailed metrics for failed apps#37120
Closed
gabrielpetry wants to merge 10 commits intorelease-7.11.0from
Closed
Chore: Adds detailed metrics for failed apps#37120gabrielpetry wants to merge 10 commits intorelease-7.11.0from
gabrielpetry wants to merge 10 commits intorelease-7.11.0from
Conversation
…w endpoint replacing it (#36973)
Co-authored-by: Aleksander Nicacio da Silva <6494543+aleksandernsilva@users.noreply.github.com>
Co-authored-by: Tasso Evangelista <2263066+tassoevan@users.noreply.github.com>
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: b3046ae The changes in this PR will be included in the next version bump. This PR includes changesets to release 42 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
|
Contributor
|
Caution Review failedThe pull request is closed. WalkthroughIntroduces a new REST endpoint livechat/inquiries.returnAsInquiry, deprecates the Meteor method, updates server validations (including MAC limit and room closed), migrates client hooks to the REST endpoint, adds AbortSignal support to endpoint calls, adjusts metrics/statistics for failed apps, updates ChatInfo tags semantics, expands tests, adds router logging, and updates i18n. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Client UI
participant Hook as useEndpoint
participant Provider as ServerProvider.callEndpoint
participant Router as HTTP Router
participant API as Livechat REST Controller
participant Domain as returnRoomAsInquiry
participant Models as LivechatRooms
UI->>Hook: returnChatToQueue({ roomId, departmentId? }, { signal? })
Hook->>Provider: callEndpoint(POST /v1/livechat/inquiries.returnAsInquiry, payload, signal?)
Provider->>Router: HTTP POST /v1/livechat/inquiries.returnAsInquiry
Router-->>Router: Validate query/body schemas
alt Validation fails
Router-->>Provider: 400 { error, details }
Provider-->>Hook: reject
Hook-->>UI: surface error
else OK
Router->>API: handle request
API->>Models: find room by roomId
alt Room not found
API-->>Router: 404/400 failure
else Found
API->>Domain: returnRoomAsInquiry(room, departmentId?)
alt MAC limit exceeded
Domain-->>API: throw 'error-mac-limit-reached'
API-->>Router: 400 mapped failure
else Room closed or other rules
Domain-->>API: throw with message ('Room closed', etc.)
API-->>Router: 400 mapped failure
else Success
Domain-->>API: true
API-->>Router: 200 { result: true }
end
end
Router-->>Provider: Response
Provider-->>Hook: resolve
Hook-->>UI: onSuccess/invalidate queries
end
sequenceDiagram
autonumber
participant Comp as Component
participant Hook as useEndpoint
participant Provider as callEndpoint
participant Net as fetch()
Note over Comp,Net: Abortable endpoint call with AbortSignal
Comp->>Hook: fn(params, { signal })
Hook->>Provider: callEndpoint(..., signal)
Provider->>Net: fetch(url, { signal })
alt signal.abort()
Net-->>Provider: AbortError
Provider-->>Hook: throw AbortError
Hook-->>Comp: handle cancellation
else completes
Net-->>Provider: Response
Provider-->>Hook: Data
Hook-->>Comp: Data
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (35)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
gabrielpetry
changed the title
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Rocket.Chat livechat inquiry return functionality by migrating from Meteor methods to REST endpoints and adds comprehensive metrics tracking for failed apps.
- Migrates
livechat:returnAsInquiryMeteor method to a new REST endpoint/v1/livechat/inquiries.returnAsInquiry - Adds detailed metrics for failed apps including names and error messages via a new Prometheus gauge
- Enhances endpoint infrastructure to support AbortSignal for request cancellation
Reviewed Changes
Copilot reviewed 35 out of 36 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/meteor/app/livechat/imports/server/rest/inquiries.ts | Implements new REST endpoint for returning inquiries with proper validation and error handling |
| apps/meteor/app/livechat/server/methods/returnAsInquiry.ts | Adds deprecation warning to existing Meteor method |
| apps/meteor/app/statistics/server/lib/getAppsStatistics.ts | Modifies statistics to return detailed failed app information instead of just counts |
| apps/meteor/app/metrics/server/lib/metrics.ts | Adds new Prometheus gauge for tracking failed app details |
| packages/ui-contexts/src/hooks/useEndpoint.ts | Adds AbortSignal support to endpoint hooks |
| multiple client files | Updates references from Meteor method to new REST endpoint |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| totalActive++; | ||
| } else if (status !== AppStatus.MANUALLY_DISABLED) { | ||
| totalFailed++; | ||
| appsFailed.push({name: app.getInfo().name, error: app.getStatus()}); |
There was a problem hiding this comment.
Missing spaces around object properties. Add spaces around the colon for consistency: { name: app.getInfo().name, error: app.getStatus() }.
Suggested change
| appsFailed.push({name: app.getInfo().name, error: app.getStatus()}); | |
| appsFailed.push({ name: app.getInfo().name, error: app.getStatus() }); |
| "Features": "Funktionen", | ||
| "Federated": "Verbunden", | ||
| "Federation": "Verbund", | ||
| "Federation_Description": "Verbund ermöglicht es einer begrenzten Anzahl von Arbeitsbereichen, miteinander zu kommunizieren.", |
There was a problem hiding this comment.
The German translation contains an error. It says 'begrenzten Anzahl' (limited number) but should say 'unbegrenzten Anzahl' (unlimited number) to match the English meaning.
Suggested change
| "Federation_Description": "Verbund ermöglicht es einer begrenzten Anzahl von Arbeitsbereichen, miteinander zu kommunizieren.", | |
| "Federation_Description": "Verbund ermöglicht es einer unbegrenzten Anzahl von Arbeitsbereichen, miteinander zu kommunizieren.", |
| metrics.totalAppsFailed.set(appsFailed.length || 0); | ||
|
|
||
| for (const app of appsFailed) { | ||
| metrics.appsFailedReason.set({ ...app }, 1); |
There was a problem hiding this comment.
Using spread operator on app assumes it has the correct structure. Consider explicitly destructuring the expected properties: metrics.appsFailedReason.set({ name: app.name, error: app.error }, 1);
Suggested change
| metrics.appsFailedReason.set({ ...app }, 1); | |
| metrics.appsFailedReason.set({ name: app.name, error: app.error }, 1); |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-7.11.0 #37120 +/- ##
==================================================
+ Coverage 67.38% 67.40% +0.02%
==================================================
Files 3328 3330 +2
Lines 113342 113502 +160
Branches 20562 20612 +50
==================================================
+ Hits 76375 76509 +134
- Misses 34363 34389 +26
Partials 2604 2604
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a new metric to track the reasons for app failures.
getAppsStatisticsto return detailed data about failed apps, including their names and error messages.rocketchat_apps_failed_reasonto expose this detailed information.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores