feat(jans-cedarling): Improve log searching and retrieval #10772

olehbozhok · 2025-01-30T21:43:00Z

Prepare

Read PR guidelines
Read license information

Description

Target issue

link

closes #10705

Implementation Details

extended API of memory logger

/// Get logs by tag, like `log_kind` or `log level`.
/// Tag can be `log_kind`, `log_level`.
fn get_logs_by_tag(&self, tag: &str) -> Vec<serde_json::Value>;

/// Get logs by request_id.
/// Return log entries that match the given request_id.
fn get_logs_by_request_id(&self, request_id: &str) -> Vec<serde_json::Value>;

/// Get log by request_id and tag, like composite key `request_id` + `log_kind`.
/// Tag can be `log_kind`, `log_level`.
/// Return log entries that match the given request_id and tag.
fn get_logs_by_request_id_and_tag(&self, request_id: &str, tag: &str)
-> Vec<serde_json::Value>;

Test and Document the changes

Static code analysis has been run locally and issues have been fixed
Relevant unit and integration tests have been added/updated
Relevant documentation has been updated if any (i.e. user guides, installation and configuration guides, technical design docs etc)

Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with docs: to indicate documentation changes or if the below checklist is not selected.

I confirm that there is no impact on the docs due to the code changes in this PR.

…rtant for the memory logger Signed-off-by: Oleh Bohzok <[email protected]>

Signed-off-by: Oleh Bohzok <[email protected]>

…it is unused Signed-off-by: Oleh Bohzok <[email protected]>

Signed-off-by: Oleh Bohzok <[email protected]>

…xisting key Signed-off-by: Oleh Bohzok <[email protected]>

Signed-off-by: Oleh Bohzok <[email protected]>

…in trait `Indexed` to not forget implement them It lead to less amount of the bugs, because of compiler highlight Signed-off-by: Oleh Bohzok <[email protected]>

Signed-off-by: Oleh Bohzok <[email protected]>

…t allows to load information from `memory` logger Signed-off-by: Oleh Bohzok <[email protected]>

…tag` to the `get_logs_by_id_and_tag` Signed-off-by: Oleh Bohzok <[email protected]>

Signed-off-by: Oleh Bohzok <[email protected]>

…ogger API Signed-off-by: Oleh Bohzok <[email protected]>

Signed-off-by: Oleh Bohzok <[email protected]>

- rename `get_log_by_request_id` to `get_logs_by_request_id` - remove in trait `LogWriter` method `log` and now use only `log_any` Signed-off-by: Oleh Bohzok <[email protected]>

…related to getting logs from memory logger by `request_id` and tags Signed-off-by: Oleh Bohzok <[email protected]>

…aling-issue-10705 Signed-off-by: Oleh Bohzok <[email protected]>

…nerator Signed-off-by: Oleh Bohzok <[email protected]>

Signed-off-by: Oleh Bohzok <[email protected]>

rmarinn · 2025-01-31T00:09:54Z

jans-cedarling/cedarling/src/log/memory_logger.rs

+    fn get_logs_by_tag(&self, tag: &str) -> Vec<serde_json::Value> {
+        self.storage
+            .lock()
+            .expect(STORAGE_MUTEX_EXPECT_MESSAGE)
+            .get_by_index_key(tag)
+            .map(|v| v.to_owned())
+            .collect()
+    }
+
+    fn get_logs_by_request_id(&self, request_id: &str) -> Vec<serde_json::Value> {
+        self.storage
+            .lock()
+            .expect(STORAGE_MUTEX_EXPECT_MESSAGE)
+            .get_by_index_key(request_id)
+            .map(|v| v.to_owned())
+            .collect()
+    }
+
+    fn get_logs_by_request_id_and_tag(
+        &self,
+        request_id: &str,
+        tag: &str,
+    ) -> Vec<serde_json::Value> {
+        let key = composite_key(request_id, tag);
+
+        self.storage
+            .lock()
+            .expect(STORAGE_MUTEX_EXPECT_MESSAGE)
+            .get_by_index_key(&key)
+            .map(|v| v.to_owned())
+            .collect()
+    }


shouldn't we return errors here instead of panicking?

From my point of view poisoned lock is not recoverable state of program, so returning error has no sense, because this error will newer be fixed. And we need reload program to fix.

Also in our case storage lock can newer be poisoned.
I think using panicking instead of return error (in this case) is a good tradeoff. Because It will newer happen.

From my point of view poisoned lock is not recoverable state of program, so returning error has no sense

My concern if the user of the Cedarling library wants to gracefully handle the situation. For example, what if the system they're building shouldn't shut down unexpectedly without logging the current state and the error?

Also in our case storage lock can newer be poisoned.

Im not sure about that since we're writing bindings to other languages that do not implement Rust's safety guarantees.

But if you think that will still never happen, then go ahead.

Hmm, in case if target does not support atomic it will panic anyway and returning PoisonError does not help. And in our situation poisoning is impossible. Because we don't use some tricky recursive function calling that tries to call same lock. So I'm pretty sure about that.

alright, then it's ok for me

rmarinn · 2025-01-31T00:34:55Z

jans-cedarling/cedarling/src/log/memory_logger.rs

-            .lock()
-            .expect(STORAGE_MUTEX_EXPECT_MESSAGE)
-            .set(&entry.get_request_id().to_string(), json);
+        let mut storage = self.storage.lock().expect(STORAGE_MUTEX_EXPECT_MESSAGE);


isn't it better if we return an error instead of calling .expect() in case the user wants some to implement a tear-down sequence in their code in case of a poisoned lock?

Could you describe how lock can be poisoned (in our case)?

In the bindings code for other languages, code might panic in undefined ways.

It probably won't happen in the rust code but I don't think we can guarantee that in the bindings code. I think we already saw that problem in the Kotlin bindings where the control over memory is limited.

Responded here
#10772 (comment)

jans-cedarling/sparkv/src/index.rs

Signed-off-by: Oleh Bohzok <[email protected]>

SafinWasi

Python side looks good to me

olehbozhok · 2025-02-04T01:30:40Z

Sorry, I slightly messed up the commits, but everything is OK now.
I’d appreciate it if you could approve it again.

olehbozhok added 25 commits January 21, 2025 01:39

chore(jans-cedarling): extend trait Loggable with methods that impo…

39a2e18

…rtant for the memory logger Signed-off-by: Oleh Bohzok <[email protected]>

chore(jans-cedarling): update to new interface

a552fe3

Signed-off-by: Oleh Bohzok <[email protected]>

chore(jans-cedarling): generate one request_id per authorize request

42d8cc3

Signed-off-by: Oleh Bohzok <[email protected]>

chore(jans-cedarling): remove PartialEq, Eq macros from KvEntry, …

a50e594

…it is unused Signed-off-by: Oleh Bohzok <[email protected]>

chore(jans-cedarling): add import entities from std lib

024469f

Signed-off-by: Oleh Bohzok <[email protected]>

chore(jans-cedarling): remove using unwrap

e9af2bc

Signed-off-by: Oleh Bohzok <[email protected]>

feat(jans-cedarling): add to sparkv API to push additional keys for e…

5e48fbb

…xisting key Signed-off-by: Oleh Bohzok <[email protected]>

chore(jans-cedarling): update traits to work with additional index

0786d4c

Signed-off-by: Oleh Bohzok <[email protected]>

feat(jans-cedarling): add methods for getting logs by tag

fabd1db

Signed-off-by: Oleh Bohzok <[email protected]>

chore(jans-cedarling): remove default implementation for some method …

459eb58

…in trait `Indexed` to not forget implement them It lead to less amount of the bugs, because of compiler highlight Signed-off-by: Oleh Bohzok <[email protected]>

chore(jans-cedarling): fix get_index_keys to be more correct

39044ac

Signed-off-by: Oleh Bohzok <[email protected]>

feat(jans-cedarling): add to AuthorizeResult field request_id tha…

2fed7e3

…t allows to load information from `memory` logger Signed-off-by: Oleh Bohzok <[email protected]>

chore(jans-cedarling): rename method `LogStorage::get_logs_by_id_and_…

30d5281

…tag` to the `get_logs_by_id_and_tag` Signed-off-by: Oleh Bohzok <[email protected]>

feat(jans-cedarling): add method get_log_by_request_id

6db5aa8

Signed-off-by: Oleh Bohzok <[email protected]>

chore(jans-cedarling): update python bindings related to new memory l…

f75fd62

…ogger API Signed-off-by: Oleh Bohzok <[email protected]>

feat(jans-cedarling): update python pyi file

65c6da2

Signed-off-by: Oleh Bohzok <[email protected]>

test(jans-cedarling): add python tests to load data from memory logger

f7dbc97

Signed-off-by: Oleh Bohzok <[email protected]>

chore(jans-cedarling): minor changes with naming

b618e69

- rename `get_log_by_request_id` to `get_logs_by_request_id` - remove in trait `LogWriter` method `log` and now use only `log_any` Signed-off-by: Oleh Bohzok <[email protected]>

feat(jans-cedarling):update WASM bindings, add new fields and method …

13b7242

…related to getting logs from memory logger by `request_id` and tags Signed-off-by: Oleh Bohzok <[email protected]>

Merge commit 'ba5de5b3291601052de0a932cf8f786c16af26bf' into jans-ced…

d801f2c

…aling-issue-10705 Signed-off-by: Oleh Bohzok <[email protected]>

chore(jans-cedarling): add clippy rule to not use standart uuid7 ge…

cae939c

…nerator Signed-off-by: Oleh Bohzok <[email protected]>

chore(jans-cedarling): fix clippy issues

3b8a760

Signed-off-by: Oleh Bohzok <[email protected]>

chore(jans-cedarling): fix unit tests when compare id of log entry

b3ab07d

Signed-off-by: Oleh Bohzok <[email protected]>

test(jans-cedarling): add test logs for memory logger index

2dcc8ac

Signed-off-by: Oleh Bohzok <[email protected]>

docs(jans-cedarling): update logger docs

7076012

Signed-off-by: Oleh Bohzok <[email protected]>

olehbozhok requested review from nynymike, SafinWasi, duttarnab and rmarinn January 30, 2025 21:43

mo-auto added the area-documentation Documentation needs to change as part of issue or PR label Jan 30, 2025

mo-auto added comp-docs Touching folder /docs comp-jans-cedarling Touching folder /jans-cedarling kind-feature Issue or PR is a new feature request labels Jan 30, 2025

olehbozhok changed the title ~~feat(jans-cedarling): improve getting logs from memory logger~~ feat(jans-cedarling): Improve log searching and retrieval Jan 30, 2025

olehbozhok added 2 commits January 31, 2025 00:16

test(jans-cedarling): fix python tests

eaabb0f

Signed-off-by: Oleh Bohzok <[email protected]>

docs(jans-cedarling): update python documentation

46b321f

Signed-off-by: Oleh Bohzok <[email protected]>

olehbozhok self-assigned this Jan 30, 2025

rmarinn reviewed Jan 31, 2025

View reviewed changes

olehbozhok added 2 commits January 31, 2025 16:37

test(jans-cedarling): add test cases for sparkv index

bec069c

Signed-off-by: Oleh Bohzok <[email protected]>

Merge branch 'main' into jans-cedaling-issue-10705

823fe70

SafinWasi previously approved these changes Jan 31, 2025

View reviewed changes

nynymike previously approved these changes Feb 3, 2025

View reviewed changes

This was referenced Feb 3, 2025

feat(jans-cedarling): isolate decision logs for sidecar #10785

Open

feat(jans-cedarling): update sidecar for log retrieval #10786

Open

olehbozhok dismissed stale reviews from nynymike and SafinWasi via 06d3032 February 3, 2025 21:55

olehbozhok force-pushed the jans-cedaling-issue-10705 branch from 06d3032 to 823fe70 Compare February 4, 2025 01:27

rmarinn approved these changes Feb 4, 2025

View reviewed changes

duttarnab approved these changes Feb 4, 2025

View reviewed changes

olehbozhok enabled auto-merge (squash) February 4, 2025 16:19

Merge branch 'main' into jans-cedaling-issue-10705

8d6963e

olehbozhok merged commit 9286f82 into main Feb 4, 2025
1 of 2 checks passed

olehbozhok deleted the jans-cedaling-issue-10705 branch February 4, 2025 16:20

mo-auto mentioned this pull request Feb 10, 2025

chore(main): release 1.4.0 #10818

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(jans-cedarling): Improve log searching and retrieval #10772

feat(jans-cedarling): Improve log searching and retrieval #10772

olehbozhok commented Jan 30, 2025 •

edited

Loading

rmarinn Jan 31, 2025

olehbozhok Jan 31, 2025

rmarinn Feb 1, 2025

olehbozhok Feb 3, 2025

rmarinn Feb 4, 2025

rmarinn Jan 31, 2025

olehbozhok Jan 31, 2025

rmarinn Feb 1, 2025 •

edited

Loading

olehbozhok Feb 3, 2025

SafinWasi left a comment

olehbozhok commented Feb 4, 2025

feat(jans-cedarling): Improve log searching and retrieval #10772

feat(jans-cedarling): Improve log searching and retrieval #10772

Conversation

olehbozhok commented Jan 30, 2025 • edited Loading

Prepare

Description

Target issue

Implementation Details

Test and Document the changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmarinn Feb 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SafinWasi left a comment

Choose a reason for hiding this comment

olehbozhok commented Feb 4, 2025

olehbozhok commented Jan 30, 2025 •

edited

Loading

rmarinn Feb 1, 2025 •

edited

Loading