INC-624: Add logs when API requests are slow #2154
Open
Conversation
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
🦋 Changeset detectedLatest commit: f11b79b The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2154 +/- ##
==========================================
- Coverage 36.07% 36.03% -0.05%
==========================================
Files 938 940 +2
Lines 60362 60458 +96
Branches 2968 2972 +4
==========================================
+ Hits 21778 21784 +6
- Misses 38224 38313 +89
- Partials 360 361 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive logging and tracing for external API calls to help diagnose slow API requests and timeouts. The implementation introduces a reusable traceExternalCall utility that wraps async operations with timing instrumentation and conditional warning logs when calls exceed configured thresholds.
Key changes:
- New
traceExternalCallutility function that logs start/finish times and warns on slow operations - Systematic wrapping of all Saleor GraphQL, Algolia, and metadata operations with tracing
- Fixed a missing
awaitbug insetFieldsMappingConfigmutation (line 113 in configuration.router.ts)
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/search/src/lib/trace-external-calls.ts | New utility providing timing instrumentation with configurable thresholds (Algolia: 10s, Saleor: 5s, DynamoDB: 1s) |
| apps/search/src/webhooks/webhook-context.ts | Wrapped channel fetching GraphQL query with tracing |
| apps/search/src/pages/api/webhooks-status.ts | Added tracing to webhook fetching API endpoint |
| apps/search/src/pages/api/setup-indices.ts | Wrapped metadata and channel queries with tracing in parallel Promise.all |
| apps/search/src/modules/configuration/configuration.router.ts | Added tracing to all config get/set operations and fixed missing await on line 113 |
| apps/search/src/lib/algolia/getAlgoliaConfiguration.ts | Wrapped metadata retrieval with tracing |
| apps/search/src/lib/algolia/algoliaSearchProvider.ts | Added tracing to all Algolia operations (saveObjects, deleteObjects, setSettings, deleteBy) and improved variable naming (objects → objectIds) |
| apps/search/src/lib/algolia/algolia-credentials-verifier.ts | Wrapped API key verification with tracing |
| apps/search/src/domain/WebhookActivityToggler.service.ts | Added tracing to all webhook management operations (fetch, enable, disable, create, remove) |
| .changeset/spotty-mirrors-sort.md | Documented the new tracing feature with threshold values for each service type |
lkostrowski
reviewed
Dec 10, 2025
lkostrowski
reviewed
Dec 10, 2025
Member
lkostrowski
left a comment
lkostrowski
left a comment
There was a problem hiding this comment.
In generale LGTM, but I suggest to invest a little to make it even better
- Rename it to
traceEffector something like this, because it doesn't really have to be external. It is universal utility and you can wrap also loading file from disk or or long computation - ...Which makes it our own custom wrapper on trace with extra sugar in the future(we can add extra params, eg "errorOnTimeout: true" which can set attribute etc)
- we can already put it to shared package because we definitely should wrap other apps with it
and I think signature will be better with more factory-like:
const traceEffect = createTraceEffect({...});
traceEffect(() => apiCall(...))
because you can reuse them
Contributor
Differences Found✅ No packages or licenses were added. SummaryExpand
|
Comment on lines
+89
to
+90
| return this.traceCreateWebhook(() => | ||
| this.client.mutation(CreateWebhookDocument, { input }).toPromise(), |
There was a problem hiding this comment.
[nitpick] The createWebhook trace call doesn't pass any attributes, while other methods in this class pass relevant IDs for observability (e.g., webhookId, appId). Consider adding non-sensitive attributes like the webhook's target URL or event type from the input to improve log correlation and debugging capabilities.
Suggested change
| return this.traceCreateWebhook(() => | |
| this.client.mutation(CreateWebhookDocument, { input }).toPromise(), | |
| return this.traceCreateWebhook( | |
| () => this.client.mutation(CreateWebhookDocument, { input }).toPromise(), | |
| { | |
| targetUrl: input.targetUrl, | |
| eventType: Array.isArray(input.asyncEvents) && input.asyncEvents.length > 0 | |
| ? input.asyncEvents.join(",") | |
| : Array.isArray(input.syncEvents) && input.syncEvents.length > 0 | |
| ? input.syncEvents.join(",") | |
| : undefined, | |
| }, |
| "version": "2.3.1", | ||
| "type": "module", | ||
| "exports": { | ||
| "./trace-effect": "./src/trace-effect.ts", |
There was a problem hiding this comment.
[nitpick] Inconsistent export path pattern: The new trace-effect export uses "./trace-effect": "./src/trace-effect.ts" while all existing exports use the pattern "./src/...": "./src/....ts". For consistency, consider either:
- Changing this to
"./src/trace-effect": "./src/trace-effect.ts"to match existing exports, or - Creating a cleaner export like
"./trace-effect"is fine, but then update imports in consuming code to use the same pattern as other imports (e.g., they currently use@saleor/apps-otel/src/with-span-attributes).
Suggested change
| "./trace-effect": "./src/trace-effect.ts", | |
| "./src/trace-effect": "./src/trace-effect.ts", |
Co-authored-by: Copilot <[email protected]>
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.
Added new utility to trace effects (e.g. external requests). It wraps it with OTEL span and adds logs when we exceed our treshold.
It doesn't cancel request itself, it's used only for observability for now, but can be extended later.
It adds two separate methods: