Skip to content

Add input flavors (raw, json-to-messagepack, and json)#252

Merged
makrisoft merged 4 commits intomainfrom
ernie/binary
Mar 26, 2024
Merged

Add input flavors (raw, json-to-messagepack, and json)#252
makrisoft merged 4 commits intomainfrom
ernie/binary

Conversation

@makrisoft
Copy link
Contributor

@makrisoft makrisoft commented Mar 7, 2024

#gsd:39011

  • Drop the log buffer altogether and only warn when the log length will be greater than the current limit:
image
  • Added an Codec to the runner so that input can be passed as:
    • json (default for today)
    • binary untouched passthrough (helps us test argo, messagepack etc)
    • messagepack json -> messagepack transcoding (adds rmp-serde as a new dependency)

This pull request introduces several changes to the src/engine.rs, src/function_run_result.rs, src/logs.rs, src/main.rs, tests/fixtures/log_truncation_function/src/main.rs and Cargo.toml files. The most significant changes include the addition of a new library, the restructuring of function parameters into a struct, changes to error handling, and updates to test cases.

Dependency Changes:

  • Cargo.toml: Added rmp-serde = "1.1" to the list of dependencies. This library provides serialization and deserialization between Rust and the MessagePack format.

Function Parameter Restructuring:

  • src/engine.rs: The parameters of the run function were moved into a new struct called FunctionRunParams. This change simplifies the function's signature and makes it easier to add, remove, or modify parameters in the future.

Error Handling and Logging:

  • src/engine.rs: Removed the .expect("Couldn't append error logs"); call when appending error logs, which simplifies error handling.
  • src/function_run_result.rs: Added a check for the length of logs and a warning message if the logs exceed a certain limit.
  • src/logs.rs: Removed the capacity limit for LogStream and the associated truncation logic. This change simplifies the log handling process. [1] [2] [3] [4]

Test Case Updates:

  • src/engine.rs and tests/fixtures/log_truncation_function/src/main.rs: Updated test cases to reflect the changes in function parameters and log handling. [1] [2] [3]

Input Handling:

  • src/main.rs: Added support for different input codecs, including JSON, raw input, and JSON converted to MessagePack. This change adds flexibility in how input data can be provided. [1] [2] [3]

@makrisoft makrisoft self-assigned this Mar 7, 2024
@makrisoft makrisoft requested a review from jbourassa March 11, 2024 18:50
src/main.rs Outdated
Comment on lines +64 to +65
#[clap(short = 'l', long, value_enum, default_value = "json")]
input_flavor: InputFlavor,
Copy link
Contributor

Choose a reason for hiding this comment

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

How did you pick l?

I've been thinking we could call this codec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the next letter after f in flavor :P
codec is better! thanks!

src/main.rs Outdated
Comment on lines +67 to +69
/// Log buffer size. Specify either a number or "unbounded" to disable the limit.
#[clap(short = 'b', long, default_value = "1000")]
log_buffer: LogMaxSize,
Copy link
Contributor

Choose a reason for hiding this comment

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

We could probably drop the max altogether, and emit a warning if the log would've beeb truncated in production? Not sure when log truncation is ever desirable here.

@makrisoft makrisoft requested a review from jbourassa March 20, 2024 22:11
Copy link
Contributor

@jbourassa jbourassa left a comment

Choose a reason for hiding this comment

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

LGTM outside the 20kb -> 1kb log limit change.

Comment on lines +5 to +6
const FUNCTION_LOG_LIMIT: usize = 20_000;

Copy link
Contributor

Choose a reason for hiding this comment

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

Logs are at most 1kB (1,000 bytes), from the docs:

Function logs to STDERR are truncated after 1 kB. Some Wasm toolchains might crash if STDERR fails to write full logs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oof! I confused it with output :)
I'll fix, thanks!


const PROFILE_DEFAULT_INTERVAL: u32 = 500_000; // every 5us

/// Supported input flavors
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be nice to extend this to include output flavor, but we can do that later.

@makrisoft makrisoft requested a review from andrewhassan March 25, 2024 14:08
@makrisoft makrisoft merged commit 4cbb9f3 into main Mar 26, 2024
@makrisoft makrisoft deleted the ernie/binary branch March 26, 2024 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants