nodejs: implement 130/311 benchmarks #281
Open
+243
−0
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
📝 WalkthroughWalkthroughTwo new benchmark modules are introduced: a CRUD API for shopping cart operations with NoSQL storage and a compression utility for archiving files with performance metrics, each with corresponding package manifests. Changes
Sequence DiagramssequenceDiagram
participant Client
participant Handler as CRUD Handler
participant NoSQL as NoSQL Layer
Client->>Handler: event (route + body)
alt PUT /cart
Handler->>NoSQL: addProduct(cartId, productId, name, price, qty)
NoSQL-->>Handler: success
else GET /cart/{id}
Handler->>NoSQL: getProducts(cartId, productId)
NoSQL-->>Handler: product entry
else GET /cart
Handler->>NoSQL: queryProducts(cartId)
NoSQL-->>Handler: all cart items
Handler->>Handler: aggregate (sum prices, avg, names)
end
Handler-->>Client: { result: [...] }
sequenceDiagram
participant Client
participant Handler as Compression Handler
participant Storage as Storage Layer
Client->>Handler: event (bucket, key)
Handler->>Storage: download directory
Storage-->>Handler: file contents
Handler->>Handler: compute dir size
Handler->>Handler: create tar.gz in-memory
Handler->>Handler: gzip compress
Handler->>Storage: upload archive
Storage-->>Handler: confirmation
Handler-->>Client: { bucket, key, timings, sizes }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@benchmarks/100.webapps/130.crud-api/nodejs/function.js`:
- Around line 30-46: The code currently computes priceSum by adding
product.price per product, ignoring product.quantity; update the loop that
builds products, priceSum and quantitySum so that priceSum accumulates
product.price * product.quantity (i.e., total cost), keep quantitySum +=
product.quantity, and then compute avgPrice as (quantitySum > 0 ? priceSum /
quantitySum : 0.0) so avg_price is the correct per-item average; return
products, total_cost: priceSum (now total cost), and avg_price: avgPrice.
In `@benchmarks/300.utilities/311.compression/nodejs/function.js`:
- Around line 41-88: The function createTarGzArchive builds a custom
length-prefixed binary stream and then gzips it but still treats it as a .tar.gz
(outputPath), which is misleading and incompatible with tar tools; either (A)
rename the function and ensure outputPath uses a custom extension (e.g., change
createTarGzArchive to createCustomGzArchive and enforce outputPath
endsWith('.bin.gz') or '.custom.gz') so callers don't expect a real tar, or (B)
make it produce a real tar by replacing the custom packing logic with a proper
tar implementation (e.g., use the 'tar' module or node-tar to pack files from
sourceDir into a tar stream and then gzip it) and keep the .tar.gz extension;
update the function signature and any callers that reference createTarGzArchive
or outputPath accordingly.
- Around line 90-146: The handler creates a temporary directory at download_path
but never removes it; update exports.handler to wrap the
download/compress/upload logic in try/finally (or try/catch/finally) and in the
finally block synchronously or asynchronously remove the download_path (use
fs.rm or fs.rmdir with recursive:true or a safe recursive delete helper) so the
directory is always cleaned up even on errors; ensure you reference
download_path, createTarGzArchive, storage_handler.upload and await
uploadPromise before cleanup to avoid deleting files mid-upload and catch/remove
any deletion errors silently or log them.
🧹 Nitpick comments (1)
benchmarks/100.webapps/130.crud-api/nodejs/package.json (1)
2-8: Specify a valid license (and author if applicable) to avoid compliance ambiguity.
Leaving"license"empty can create distribution/compliance uncertainty. Please set a valid SPDX identifier (or"UNLICENSED"if intentional) consistent with the repository’s policy.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
benchmarks/100.webapps/130.crud-api/nodejs/function.jsbenchmarks/100.webapps/130.crud-api/nodejs/package.jsonbenchmarks/300.utilities/311.compression/nodejs/function.jsbenchmarks/300.utilities/311.compression/nodejs/package.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-19T16:15:01.811Z
Learnt from: mcopik
Repo: spcl/serverless-benchmarks PR: 99
File: benchmarks/000.microbenchmarks/010.sleep/cpp/main.cpp:11-0
Timestamp: 2025-06-19T16:15:01.811Z
Learning: In the serverless-benchmarks project, input validation for benchmark functions is not required since the benchmarks are invoked directly by the maintainers who control the input values. This applies to functions like the sleep benchmark in benchmarks/000.microbenchmarks/010.sleep/cpp/main.cpp.
Applied to files:
benchmarks/300.utilities/311.compression/nodejs/package.json
🔇 Additional comments (3)
benchmarks/300.utilities/311.compression/nodejs/function.js (2)
1-7: LGTM!The imports are appropriate, and module-scope
storage_handlerinstantiation enables connection reuse across warm invocations in serverless environments.
14-32: LGTM!The recursive directory size calculation is correct. Based on learnings, input validation is not required since benchmarks are invoked directly by maintainers.
benchmarks/300.utilities/311.compression/nodejs/package.json (1)
6-8: No action needed. The uuid@10.0.0 package is a valid released version (published June 2024) and is stable for use.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Comment on lines
+30
to
+46
| const products = []; | ||
| let priceSum = 0; | ||
| let quantitySum = 0; | ||
|
|
||
| for (const product of res) { | ||
| products.push(product.name); | ||
| priceSum += product.price; | ||
| quantitySum += product.quantity; | ||
| } | ||
|
|
||
| const avgPrice = quantitySum > 0 ? priceSum / quantitySum : 0.0; | ||
|
|
||
| return { | ||
| products: products, | ||
| total_cost: priceSum, | ||
| avg_price: avgPrice | ||
| }; |
There was a problem hiding this comment.
Fix total_cost (and avg_price) to account for quantity.
Right now total_cost sums unit prices only, so carts with quantity > 1 are undercounted. This also skews avg_price.
🐛 Proposed fix
const products = [];
let priceSum = 0;
let quantitySum = 0;
for (const product of res) {
products.push(product.name);
- priceSum += product.price;
+ priceSum += product.price * product.quantity;
quantitySum += product.quantity;
}
const avgPrice = quantitySum > 0 ? priceSum / quantitySum : 0.0;🤖 Prompt for AI Agents
In `@benchmarks/100.webapps/130.crud-api/nodejs/function.js` around lines 30 - 46,
The code currently computes priceSum by adding product.price per product,
ignoring product.quantity; update the loop that builds products, priceSum and
quantitySum so that priceSum accumulates product.price * product.quantity (i.e.,
total cost), keep quantitySum += product.quantity, and then compute avgPrice as
(quantitySum > 0 ? priceSum / quantitySum : 0.0) so avg_price is the correct
per-item average; return products, total_cost: priceSum (now total cost), and
avg_price: avgPrice.
There was a problem hiding this comment.
🧩 Analysis chain
@ldzgch, let me check the Python implementation to verify the issue.
🏁 Script executed:
#!/bin/bash
# Find the Python implementation of the crud-api benchmark
fd -e py . benchmarks/100.webapps/130.crud-api/
Length of output: 99
🏁 Script executed:
#!/bin/bash
# Read the Python implementation to check for the same issue
cat benchmarks/100.webapps/130.crud-api/python/function.py
Length of output: 1714
@ldzgch, you're absolutely right! The Python implementation has the identical issue on line 35:
price_sum += product["price"] # Should be: product["price"] * product["quantity"]Both implementations incorrectly compute total_cost by summing unit prices instead of price × quantity, which also skews the avg_price calculation.
Would you like me to fix both the Node.js and Python implementations together? I can update:
benchmarks/100.webapps/130.crud-api/nodejs/function.js(line 35)benchmarks/100.webapps/130.crud-api/python/function.py(line 35)
Let me know and I'll prepare the changes! 🐰
Comment on lines
+41
to
+88
| async function createTarGzArchive(sourceDir, outputPath) { | ||
| // Create a simple tar-like format (concatenated files with headers) | ||
| const files = []; | ||
|
|
||
| function collectFiles(dir, baseDir = '') { | ||
| const entries = fs.readdirSync(dir); | ||
| for (const entry of entries) { | ||
| const fullPath = path.join(dir, entry); | ||
| const relativePath = path.join(baseDir, entry); | ||
| const stat = fs.statSync(fullPath); | ||
|
|
||
| if (stat.isDirectory()) { | ||
| collectFiles(fullPath, relativePath); | ||
| } else { | ||
| files.push({ | ||
| path: relativePath, | ||
| fullPath: fullPath, | ||
| size: stat.size | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| collectFiles(sourceDir); | ||
|
|
||
| // Create a concatenated buffer of all files with simple headers | ||
| const chunks = []; | ||
| for (const file of files) { | ||
| const content = fs.readFileSync(file.fullPath); | ||
| // Simple header: filename length (4 bytes) + filename + content length (4 bytes) + content | ||
| const pathBuffer = Buffer.from(file.path); | ||
| const pathLengthBuffer = Buffer.allocUnsafe(4); | ||
| pathLengthBuffer.writeUInt32BE(pathBuffer.length, 0); | ||
| const contentLengthBuffer = Buffer.allocUnsafe(4); | ||
| contentLengthBuffer.writeUInt32BE(content.length, 0); | ||
|
|
||
| chunks.push(pathLengthBuffer); | ||
| chunks.push(pathBuffer); | ||
| chunks.push(contentLengthBuffer); | ||
| chunks.push(content); | ||
| } | ||
|
|
||
| const combined = Buffer.concat(chunks); | ||
|
|
||
| // Compress using gzip | ||
| const compressed = zlib.gzipSync(combined, { level: 9 }); | ||
| fs.writeFileSync(outputPath, compressed); | ||
| } |
There was a problem hiding this comment.
Misleading archive format naming.
The function creates a custom binary format (length-prefixed files), not an actual tar archive. However, the output uses .tar.gz extension (line 110), which will fail if anyone attempts to extract it with standard tar tools.
Consider either:
- Renaming to
.bin.gzor similar custom extension - Using a proper tar library if interoperability is needed
🤖 Prompt for AI Agents
In `@benchmarks/300.utilities/311.compression/nodejs/function.js` around lines 41
- 88, The function createTarGzArchive builds a custom length-prefixed binary
stream and then gzips it but still treats it as a .tar.gz (outputPath), which is
misleading and incompatible with tar tools; either (A) rename the function and
ensure outputPath uses a custom extension (e.g., change createTarGzArchive to
createCustomGzArchive and enforce outputPath endsWith('.bin.gz') or
'.custom.gz') so callers don't expect a real tar, or (B) make it produce a real
tar by replacing the custom packing logic with a proper tar implementation
(e.g., use the 'tar' module or node-tar to pack files from sourceDir into a tar
stream and then gzip it) and keep the .tar.gz extension; update the function
signature and any callers that reference createTarGzArchive or outputPath
accordingly.
Comment on lines
+90
to
+146
| exports.handler = async function(event) { | ||
| const bucket = event.bucket.bucket; | ||
| const input_prefix = event.bucket.input; | ||
| const output_prefix = event.bucket.output; | ||
| const key = event.object.key; | ||
|
|
||
| // Create unique download path | ||
| const download_path = path.join('/tmp', `${key}-${uuidv4()}`); | ||
| fs.mkdirSync(download_path, { recursive: true }); | ||
|
|
||
| // Download directory from storage | ||
| const s3_download_begin = Date.now(); | ||
| await storage_handler.download_directory(bucket, path.join(input_prefix, key), download_path); | ||
| const s3_download_stop = Date.now(); | ||
|
|
||
| // Calculate size of downloaded files | ||
| const size = parseDirectory(download_path); | ||
|
|
||
| // Compress directory | ||
| const compress_begin = Date.now(); | ||
| const archive_name = `${key}.tar.gz`; | ||
| const archive_path = path.join(download_path, archive_name); | ||
| await createTarGzArchive(download_path, archive_path); | ||
| const compress_end = Date.now(); | ||
|
|
||
| // Get archive size | ||
| const archive_size = fs.statSync(archive_path).size; | ||
|
|
||
| // Upload compressed archive | ||
| const s3_upload_begin = Date.now(); | ||
| const [key_name, uploadPromise] = storage_handler.upload( | ||
| bucket, | ||
| path.join(output_prefix, archive_name), | ||
| archive_path | ||
| ); | ||
| await uploadPromise; | ||
| const s3_upload_stop = Date.now(); | ||
|
|
||
| // Calculate times in microseconds | ||
| const download_time = (s3_download_stop - s3_download_begin) * 1000; | ||
| const upload_time = (s3_upload_stop - s3_upload_begin) * 1000; | ||
| const process_time = (compress_end - compress_begin) * 1000; | ||
|
|
||
| return { | ||
| result: { | ||
| bucket: bucket, | ||
| key: key_name | ||
| }, | ||
| measurement: { | ||
| download_time: download_time, | ||
| download_size: size, | ||
| upload_time: upload_time, | ||
| upload_size: archive_size, | ||
| compute_time: process_time | ||
| } | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Missing temporary file cleanup.
The temporary directory created at download_path is never cleaned up. In serverless environments, /tmp space is limited (e.g., 512MB on AWS Lambda) and persists across warm invocations. Repeated invocations will accumulate files, eventually exhausting disk space.
Suggested fix: Add cleanup in a finally block
exports.handler = async function(event) {
const bucket = event.bucket.bucket;
const input_prefix = event.bucket.input;
const output_prefix = event.bucket.output;
const key = event.object.key;
// Create unique download path
const download_path = path.join('/tmp', `${key}-${uuidv4()}`);
- fs.mkdirSync(download_path, { recursive: true });
- // Download directory from storage
- const s3_download_begin = Date.now();
- ...
- return {
- result: {
- bucket: bucket,
- key: key_name
- },
- measurement: {
- ...
- }
- };
+ try {
+ fs.mkdirSync(download_path, { recursive: true });
+
+ // Download directory from storage
+ const s3_download_begin = Date.now();
+ // ... rest of the logic ...
+
+ return {
+ result: {
+ bucket: bucket,
+ key: key_name
+ },
+ measurement: {
+ download_time: download_time,
+ download_size: size,
+ upload_time: upload_time,
+ upload_size: archive_size,
+ compute_time: process_time
+ }
+ };
+ } finally {
+ // Cleanup temporary files
+ fs.rmSync(download_path, { recursive: true, force: true });
+ }
};📝 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.
Suggested change
| exports.handler = async function(event) { | |
| const bucket = event.bucket.bucket; | |
| const input_prefix = event.bucket.input; | |
| const output_prefix = event.bucket.output; | |
| const key = event.object.key; | |
| // Create unique download path | |
| const download_path = path.join('/tmp', `${key}-${uuidv4()}`); | |
| fs.mkdirSync(download_path, { recursive: true }); | |
| // Download directory from storage | |
| const s3_download_begin = Date.now(); | |
| await storage_handler.download_directory(bucket, path.join(input_prefix, key), download_path); | |
| const s3_download_stop = Date.now(); | |
| // Calculate size of downloaded files | |
| const size = parseDirectory(download_path); | |
| // Compress directory | |
| const compress_begin = Date.now(); | |
| const archive_name = `${key}.tar.gz`; | |
| const archive_path = path.join(download_path, archive_name); | |
| await createTarGzArchive(download_path, archive_path); | |
| const compress_end = Date.now(); | |
| // Get archive size | |
| const archive_size = fs.statSync(archive_path).size; | |
| // Upload compressed archive | |
| const s3_upload_begin = Date.now(); | |
| const [key_name, uploadPromise] = storage_handler.upload( | |
| bucket, | |
| path.join(output_prefix, archive_name), | |
| archive_path | |
| ); | |
| await uploadPromise; | |
| const s3_upload_stop = Date.now(); | |
| // Calculate times in microseconds | |
| const download_time = (s3_download_stop - s3_download_begin) * 1000; | |
| const upload_time = (s3_upload_stop - s3_upload_begin) * 1000; | |
| const process_time = (compress_end - compress_begin) * 1000; | |
| return { | |
| result: { | |
| bucket: bucket, | |
| key: key_name | |
| }, | |
| measurement: { | |
| download_time: download_time, | |
| download_size: size, | |
| upload_time: upload_time, | |
| upload_size: archive_size, | |
| compute_time: process_time | |
| } | |
| }; | |
| }; | |
| exports.handler = async function(event) { | |
| const bucket = event.bucket.bucket; | |
| const input_prefix = event.bucket.input; | |
| const output_prefix = event.bucket.output; | |
| const key = event.object.key; | |
| // Create unique download path | |
| const download_path = path.join('/tmp', `${key}-${uuidv4()}`); | |
| try { | |
| fs.mkdirSync(download_path, { recursive: true }); | |
| // Download directory from storage | |
| const s3_download_begin = Date.now(); | |
| await storage_handler.download_directory(bucket, path.join(input_prefix, key), download_path); | |
| const s3_download_stop = Date.now(); | |
| // Calculate size of downloaded files | |
| const size = parseDirectory(download_path); | |
| // Compress directory | |
| const compress_begin = Date.now(); | |
| const archive_name = `${key}.tar.gz`; | |
| const archive_path = path.join(download_path, archive_name); | |
| await createTarGzArchive(download_path, archive_path); | |
| const compress_end = Date.now(); | |
| // Get archive size | |
| const archive_size = fs.statSync(archive_path).size; | |
| // Upload compressed archive | |
| const s3_upload_begin = Date.now(); | |
| const [key_name, uploadPromise] = storage_handler.upload( | |
| bucket, | |
| path.join(output_prefix, archive_name), | |
| archive_path | |
| ); | |
| await uploadPromise; | |
| const s3_upload_stop = Date.now(); | |
| // Calculate times in microseconds | |
| const download_time = (s3_download_stop - s3_download_begin) * 1000; | |
| const upload_time = (s3_upload_stop - s3_upload_begin) * 1000; | |
| const process_time = (compress_end - compress_begin) * 1000; | |
| return { | |
| result: { | |
| bucket: bucket, | |
| key: key_name | |
| }, | |
| measurement: { | |
| download_time: download_time, | |
| download_size: size, | |
| upload_time: upload_time, | |
| upload_size: archive_size, | |
| compute_time: process_time | |
| } | |
| }; | |
| } finally { | |
| // Cleanup temporary files | |
| fs.rmSync(download_path, { recursive: true, force: true }); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In `@benchmarks/300.utilities/311.compression/nodejs/function.js` around lines 90
- 146, The handler creates a temporary directory at download_path but never
removes it; update exports.handler to wrap the download/compress/upload logic in
try/finally (or try/catch/finally) and in the finally block synchronously or
asynchronously remove the download_path (use fs.rm or fs.rmdir with
recursive:true or a safe recursive delete helper) so the directory is always
cleaned up even on errors; ensure you reference download_path,
createTarGzArchive, storage_handler.upload and await uploadPromise before
cleanup to avoid deleting files mid-upload and catch/remove any deletion errors
silently or log them.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
split out from the cloudflare PR, as requested
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.