apollo_infra: replace error JSON field with message in trace logs

[victorkstarkware]

No description provided.

[reviewable-StarkWare]

This change is 

[victorkstarkware]

• apollo_infra: replace error JSON field with message in trace logs #11524 👈 (View in Graphite)
• main-v0.14.1-committer

This stack of pull requests is managed by Graphite. Learn more about stacking.

[Itay-Tsabary-Starkware]

@Itay-Tsabary-Starkware made 4 comments.
Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @victorkstarkware).

---

crates/apollo_infra/src/trace_util_tests.rs line 19 at r1 (raw file):

    assert_eq!(directives, "b=debug,a=info,info");
}

Can the tests be about the output log?
Check the traced_test attribute

---

crates/apollo_infra/src/trace_util_tests.rs line 72 at r1 (raw file):

    assert_eq!(parsed["message"], "the error");
    assert!(parsed.get("error").is_none());
}

That shouldn't exist, but if it does, it should leave the log line as is. Could you please change the logic accordingily?

Code quote:

#[test]
fn rename_error_to_message_overwrites_existing_message_field() {
    // If both "error" and "message" exist, "error" value overwrites "message"
    let input = br#"{"error":"the error","message":"original message"}"#;
    let output = rename_error_to_message(input).unwrap();
    let parsed: serde_json::Value = serde_json::from_slice(&output).unwrap();

    // "error" value should overwrite "message"
    assert_eq!(parsed["message"], "the error");
    assert!(parsed.get("error").is_none());
}

---

crates/apollo_infra/src/trace_util_tests.rs line 84 at r1 (raw file):

    assert_eq!(parsed["message"], "second");
    assert!(parsed.get("error").is_none());
}

How can a json dict have two keys?

Code quote:

#[test]
fn rename_error_to_message_with_duplicate_error_keys_uses_last_value() {
    // Invalid JSON with duplicate keys - serde_json keeps the LAST value
    let input = br#"{"error":"first","error":"second"}"#;
    let output = rename_error_to_message(input).unwrap();
    let parsed: serde_json::Value = serde_json::from_slice(&output).unwrap();

    // serde_json keeps the last "error" value, which then gets renamed to "message"
    assert_eq!(parsed["message"], "second");
    assert!(parsed.get("error").is_none());
}

---

crates/apollo_infra/src/trace_util.rs line 1 at r1 (raw file):

use std::io::{self, Write};

Avoid this import please

Code quote:

self

[victorkstarkware]

@victorkstarkware made 4 comments.
Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @Itay-Tsabary-Starkware).

---

crates/apollo_infra/src/trace_util.rs line 1 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Avoid this import please

Done.

---

crates/apollo_infra/src/trace_util_tests.rs line 19 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Can the tests be about the output log?
Check the traced_test attribute

i couldn't find a way to use tracing_test with a custom subscriber as we use. it is possible to write more boilerplate in the tests file to test the actual log output (but then how do we test the new boilerplate code?)

---

crates/apollo_infra/src/trace_util_tests.rs line 72 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

That shouldn't exist, but if it does, it should leave the log line as is. Could you please change the logic accordingily?

Done.

---

crates/apollo_infra/src/trace_util_tests.rs line 84 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

How can a json dict have two keys?

only if serde_json fails spectacularly, which we shouldn't check for. removed.

[Itay-Tsabary-Starkware]

@Itay-Tsabary-Starkware made 1 comment and resolved 3 discussions.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @victorkstarkware).

---

crates/apollo_infra/src/trace_util_tests.rs line 19 at r1 (raw file):

Previously, victorkstarkware wrote…

i couldn't find a way to use tracing_test with a custom subscriber as we use. it is possible to write more boilerplate in the tests file to test the actual log output (but then how do we test the new boilerplate code?)

My take on this is to have a test for the log itself as well, not just the manipulation function. This is the end game here, and imo merits an adequate test.

It might require some boiler plate code, sure, e.g., an annotated with instrument err fn, but that's great, this is what we would like to test.

So please keep the current tests, but also add one for the actual log.

[Itay-Tsabary-Starkware]

@Itay-Tsabary-Starkware made 1 comment.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @victorkstarkware).

---

crates/apollo_infra/src/trace_util_tests.rs line 29 at r2 (raw file):

    assert!(!output_str.contains(r#""error""#), "got: {output_str}");
}

WDYT about a test that checks a payload (i.e., a json value) equal error is not modified?