Skip to content

WIP: collect metadata for each task #32

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rudokemper merged 4 commits into from
Feb 7, 2024
Merged

WIP: collect metadata for each task #32

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
694bc1c
WIP: collect task metadata
rudokemper Feb 5, 2024
3ea75bc
Revise error handling and naming
rudokemper Feb 7, 2024
f9425a9
Fix for var returning in generateMBTiles
rudokemper Feb 7, 2024
c79dff7
Fix for retrieving filesize
rudokemper Feb 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/azure_queue_service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,8 @@ const processQueueMessages = async () => {

const outputDir = "/maps";

// Pass the extracted values to initiateRendering
await initiateRendering(
// Pass the extracted values to initiateRendering, and receive the metadata
const metadata = await initiateRendering(
Copy link
Copy Markdown
Contributor

@IamJeffG IamJeffG Feb 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: choose a more specific word. "metadata" can be used for all sorts of stuff.

idea: taskResult, renderResult

style,
null,
null,
Expand All @@ -78,6 +78,8 @@ const processQueueMessages = async () => {
outputFilename,
);

console.log("metadata", metadata);

// Send completion message
const completionMessage = `Finished rendering map`;
await destinationQueueClient.sendMessage(completionMessage);
Expand Down
5 changes: 4 additions & 1 deletion src/cli.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ console.log("Output directory: %j", outputDir);
console.log("Output MBTiles filename: %j", outputFilename);
console.log("------------------------------------------------------");

initiateRendering(
const metadata = await initiateRendering(
style,
styleDir,
sourceDir,
Expand All @@ -119,3 +119,6 @@ initiateRendering(
outputDir,
outputFilename,
);

// output the metadata to console
console.log("Task metadata:", metadata);
2 changes: 1 addition & 1 deletion src/download_resources.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ const downloadOnlineTiles = async (
throw new Error("Invalid bounds provided");
}

let sourceUrl, sourceAttribution, sourceName;
let sourceUrl, sourceAttribution, sourceName, sourceFormat;
switch (style) {
case "google":
sourceUrl = `https://mt0.google.com/vt?lyrs=s&x={x}&y={y}&z={z}`;
Expand Down
65 changes: 40 additions & 25 deletions src/generate_resources.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
validateMinMaxValues,
} from "./tile_calculations.js";
import { renderTile } from "./render_map.js";
import { handleError } from "./utils.js";

// Generate a MapGL style JSON object from a remote source
// and an additional source.
Expand Down Expand Up @@ -121,6 +122,8 @@ export const generateMBTiles = async (
const tempPath = `${tempDir}/${outputFilename}.mbtiles`;
console.log(`Generating MBTiles file: ${tempPath}`);

let numberOfTiles = 0;

// Create a new MBTiles file
const mbtiles = await new Promise((resolve, reject) => {
new MBTiles(`${tempPath}?mode=rwc`, (err, mbtiles) => {
Expand Down Expand Up @@ -197,6 +200,9 @@ export const generateMBTiles = async (
mbtiles.putTile(zoom, x, y, tileBuffer, (err) => {
if (err) throw err;
});

// Increment the number of tiles
numberOfTiles++;
} catch (error) {
console.error(`Error rendering tile ${zoom}/${x}/${y}: ${error}`);
}
Expand All @@ -208,38 +214,47 @@ export const generateMBTiles = async (
await new Promise((resolve, reject) => {
mbtiles.stopWriting((err) => {
if (err) reject(err);
console.log(
`\x1b[32m${outputFilename}.mbtiles has been successfully generated!\x1b[0m`,
);
resolve();
});
});
} finally {
// Move the generated MBTiles file to the output directory
const outputPath = `${outputDir}/${outputFilename}.mbtiles`;
} catch (err) {
return handleError(err, "writing to MBTiles file");
}

try {
const readStream = fs.createReadStream(tempPath);
const writeStream = fs.createWriteStream(outputPath);
// Move the generated MBTiles file to the output directory
const outputPath = `${outputDir}/${outputFilename}.mbtiles`;

readStream.on("error", (err) => {
console.error(`Error reading MBTiles file: ${err}`);
});
try {
const readStream = fs.createReadStream(tempPath);
const writeStream = fs.createWriteStream(outputPath);

writeStream.on("error", (err) => {
console.error(`Error writing MBTiles file: ${err}`);
});
readStream.on("error", (err) => {
console.error(`Error reading MBTiles file: ${err}`);
});

writeStream.on("close", () => {
// Delete the temporary tiles directory and style
if (tempDir !== null) {
fs.promises.rm(tempDir, { recursive: true });
}
});
writeStream.on("error", (err) => {
console.error(`Error writing MBTiles file: ${err}`);
});

readStream.pipe(writeStream);
} catch (err) {
throw new Error(`Error moving MBTiles file: ${err}`);
}
writeStream.on("close", () => {
// Delete the temporary tiles directory and style
if (tempDir !== null) {
fs.promises.rm(tempDir, { recursive: true });
}
});

readStream.pipe(writeStream);
} catch (err) {
return handleError(err, "moving MBTiles file to output directory");
}

// Return metadata with success status
return {
status: "success",
errorCode: null,
errorMessage: null,
filename: `${outputFilename}.mbtiles`,
filesize: fs.statSync(outputPath).size,
numberOfTiles,
};
};
95 changes: 72 additions & 23 deletions src/initiate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import path from "path";

import { generateStyle, generateMBTiles } from "./generate_resources.js";
import { requestOnlineTiles } from "./download_resources.js";
import { handleError } from "./utils.js";

const MBTILES_REGEXP = /mbtiles:\/\/(\S+?)(?=[/"]+)/gi;

Expand All @@ -23,36 +24,54 @@ export const initiateRendering = async (
) => {
console.log("Initiating rendering...");

const workBegun = new Date().toISOString();

const tempDir = path.join(os.tmpdir(), "mapgl-tile-renderer-temp");
if (!fs.existsSync(tempDir)) {
fs.mkdirSync(tempDir, { recursive: true });
try {
fs.mkdirSync(tempDir, { recursive: true });
} catch (error) {
return handleError(error, "creating the temporary directory");
}
}
let stylePath = null;
let styleObject = null;

// If the style is self-hosted, let's read the style from the file.
if (style === "self") {
stylePath = path.resolve(process.cwd(), styleDir);
styleObject = JSON.parse(fs.readFileSync(stylePath, "utf-8"));
try {
styleObject = JSON.parse(fs.readFileSync(stylePath, "utf-8"));
} catch (error) {
return handleError(error, "reading the style file");
}
}

// If the style is not self-hosted, let's generate everything that we need to render tiles.
if (style !== "self") {
// Download tiles from the online source
await requestOnlineTiles(
style,
apiKey,
mapboxStyle,
monthYear,
bounds,
minZoom,
maxZoom,
tempDir,
);
try {
await requestOnlineTiles(
style,
apiKey,
mapboxStyle,
monthYear,
bounds,
minZoom,
maxZoom,
tempDir,
);
} catch (error) {
return handleError(error, "downloading tiles from the online source");
}

// Save the overlay GeoJSON to a file, if provided
if (overlay) {
fs.writeFileSync(`${tempDir}/sources/overlay.geojson`, overlay);
try {
fs.writeFileSync(`${tempDir}/sources/overlay.geojson`, overlay);
} catch (error) {
return handleError(error, "saving the overlay GeoJSON to a file");
}
console.log(`Overlay GeoJSON saved to file!`);
}

Expand All @@ -66,12 +85,16 @@ export const initiateRendering = async (

// Generate and save a stylesheet from the online source and overlay source.
if (styleObject === null) {
styleObject = generateStyle(style, overlay, tileSize);
fs.writeFileSync(
`${tempDir}/style.json`,
JSON.stringify(styleObject, null, 2),
);
console.log("Stylesheet generated and saved!");
try {
styleObject = generateStyle(style, overlay, tileSize);
fs.writeFileSync(
`${tempDir}/style.json`,
JSON.stringify(styleObject, null, 2),
);
console.log("Stylesheet generated and saved!");
} catch (error) {
return handleError(error, "generating and saving the stylesheet");
}
}

sourceDir = `${tempDir}/sources`;
Expand All @@ -82,7 +105,7 @@ export const initiateRendering = async (
if (localMbtilesMatches && !sourceDir) {
const msg =
"Stylesheet has local mbtiles file sources, but no sourceDir is set";
throw new Error(msg);
return handleError(new Error(msg), "checking for local mbtiles sources");
Copy link
Copy Markdown
Contributor

@IamJeffG IamJeffG Feb 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an example of a bad request, right? The caller has to change something before this will ever succeed.

I might define (in utils.js) two separate error handlers, one for bad client request, one for unexpected server error. The two would set different errorCode (as you have it now) or different status (per an idea I share in src/utils.js).

A minor variation of this is to define two different Error subclasses and you only throw BadRequestError(msg) here, leaving the entry point (clip.js or azure_queue_service.js) to catch that error if it wants. Example in Python:

}

if (localMbtilesMatches) {
Expand All @@ -99,13 +122,16 @@ export const initiateRendering = async (
name,
ext: ".mbtiles",
})} in stylesheet is not found in: ${path.resolve(sourceDir)}`;
throw new Error(msg);
return handleError(
new Error(msg),
"checking for local mbtiles sources",
);
}
});
}

try {
await generateMBTiles(
let metadata = await generateMBTiles(
styleObject,
styleDir,
sourceDir,
Expand All @@ -116,8 +142,31 @@ export const initiateRendering = async (
outputDir,
outputFilename,
);

console.log(
`\x1b[32m${outputFilename}.mbtiles has been successfully generated!\x1b[0m`,
);

// if successful, return the metadata
return {
style: style,
status: metadata.status,
errorCode: metadata.errorCode,
errorMessage: metadata.errorMessage,
filename: metadata.filename,
Copy link
Copy Markdown
Contributor

@IamJeffG IamJeffG Feb 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this going to be an absolute or relative path?

If absolute, is it relative to the local volume mount? A URL?

I think I might return a relative path from under the volume mount (i.e. not including the volume mount top level dir); that way the webapp server and this task worker might use different local mount locations, but below that the relative path works in both.

An Azure storage URL might be more ideal except that technically the task worker doesn't know about Azure Storage; that's why I might lean away from it.

Copy link
Copy Markdown
Member Author

@rudokemper rudokemper Feb 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, relative path. GCV works the same way - we provide the mount location as an env var, and we can do similar for the MapPacker front end.

filesize: metadata.filesize,
numberOfTiles: metadata.numberOfTiles,
numberOfAttempts: 1,
Copy link
Copy Markdown
Contributor

@IamJeffG IamJeffG Feb 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking your code does not need to handle retries explicitly; rather the queuing system will retry to deliver the same message if it hadn't succeeded (been deleted from the queue by the task worker) within the lease time.

All your task worker code has to do is read DequeuedMessageItem.dequeueCount:

const response = await sourceQueueClient.receiveMessages({
      visibilityTimeout: 2 * 60 * 60,
});
const message = response.receivedMessageItems[0];
const numberOfAttempts = message.dequeueCount;

workBegun,
workEnded: new Date().toISOString(),
Copy link
Copy Markdown
Contributor

@IamJeffG IamJeffG Feb 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd report workBegun and workEnded even if there's error.

Copy link
Copy Markdown
Contributor

@IamJeffG IamJeffG Feb 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know, maybe azure_queue_service.js should handle workBegun and workEnded. That way it could write workBegun to the DB even before the work is finished; plus you get for free the workEnded addition even in case of an error.

OTOH this would not be so good if it's important that the CLI keep track of these too.

Copy link
Copy Markdown
Member Author

@rudokemper rudokemper Feb 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think it could be interesting that we return these for the CLI. I say this from first-hand experience of running similar Node tools overnight, and not being sure when it actually finished.

// if status = success, set expiration for one year
expiration:
Copy link
Copy Markdown
Contributor

@IamJeffG IamJeffG Feb 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have any mechanism (as of now) to actually clean up expired records?

If not, I'd be inclined to leave expiration out of this, and maybe add it later once we know how that cleanup is going to work. For example, I could envision an implementation in which some "garbage collector" runs periodically, and it knows the business rules of when to clean up a record. For that matter, we may want to change those rules at some point. In both those cases, the expiration need not be set at task finish time, but rather at expiration time.

The one reason I can think of to include this right now is if the expiration will need to be surfaced in a UI well in advance.

Copy link
Copy Markdown
Member Author

@rudokemper rudokemper Feb 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No mechanism as of yet. I was thinking of our discussion on the MapPacker design doc to expire offline maps, and that the best opportunity for us to set an expiration timestamp would be at this stage of task finishing. But we could definitely leave this out until we're ready to tackle cleanup as a separate batch of work.

metadata.status === "success"
? new Date(Date.now() + 31556952000).toISOString()
: null,
};
} catch (error) {
Copy link
Copy Markdown
Contributor

@IamJeffG IamJeffG Feb 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not super experienced with running node scripts from the command line but my guess is that the user experience (for a technical user, anyway) might actually be better if you never catch the exception, if one was thrown. My guess is that, when running from CLI, the script will print the entire stack trace, as well as the error message, and this will be much more helpful for the technical developer to spot what went wrong and how to fix it.

In other words, I'm suggesting what if handleError was not called in initiate, but instead only in azure_queue_service.js? And the try/catch likewise lives in azure_queue_service.js, not here in initiateRendering(). I'm suggesting that each entrypoint has different optimal behavior of how to share error back with the caller.

(I am assuming that CLI will not ever write back to the SQL DB)

Copy link
Copy Markdown
Member Author

@rudokemper rudokemper Feb 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fully agree with you on this point. I'll push up a commit with a revision of when handleError is called (or not).

throw new Error("Error generating MBTiles:", error);
return handleError(error, "generating MBTiles file");
}
};

Expand Down
8 changes: 8 additions & 0 deletions src/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,14 @@ export const raiseError = (msg) => {
process.exit(1);
};

export const handleError = (error, message) => {
return {
status: "failed",
errorCode: "500",
Copy link
Copy Markdown
Contributor

@IamJeffG IamJeffG Feb 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know I alluded to 4xx and 5xx codes being used to differentiate between "caller's fault" vs "my fault", but I was thinking of it as an analogy. Since there is no HTTP call, "code 500" feels a bit too literal?

You could probably even omit errorCode and overload "status" with possible values of

* BadRequest
* InternalError
* Pending
* Success

Note that I would expect the webapp's /status/<taskid> endpoint to always return HTTP 200 (as long as the taskid is valid), even if the task failed or is pending, because it's successfully returning the task status. So the errorCode set here wouldn't actually end up exposed there.

That said, what you have seems clear enough that I'm also fine if you want to leave it.

Copy link
Copy Markdown
Member Author

@rudokemper rudokemper Feb 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I too was thinking of these codes as an analogy, and wasn't sure yet where to actually go with the error codes - I would have likely rewritten them to be something more accurate on the front end anyway, and I think your proposal to expand the range of possibilities in status is a better solution 👍

errorMessage: `Error ${message}: ${error.message}`,
};
};

// Currently supported list of online styles
// When adding a new style, make sure to update this list
const validOnlineStyles = [
Expand Down