Read from the audit database #5038

nickgerace · 2024-11-27T23:24:22Z

Description

This PR ensures that we read from the audit database for the audit logs interface rather than directly from the NATS JetStream stream. This is a drop-in replacement that does not result in a change to the front end. The user experience, however, should be vastly improved with one caveat that we'll cover at the end.

To accomplish this, we always read from the database in SDF and in integration tests, but enabling the audit logs app for forklift will still be opt-in for production use cases. However, for CI integration tests, local integration tests and development builds with buck2 and cargo, forklift will have the audit logs app enabled by default.

This commit removes the ability to read from the stream in the dal wholesale and also uses the new "AuditLogRow" struct and its methods to read from the database. We rely on the compound index for the "audit_logs" table in the audit database for quick querying.

Context

This switchover was implemented for a variety of reasons, including but not limited to the following:

Data correctness in the frontend (e.g. the number of rows you ask for should now be what you get, or at least however many exist for your currently selected change set)
Performance
Rich query semantics
Data segregation by database

Test Related Refactoring

Ever wondered why our services often have a "from_config" method that calls a "from_services" method? It's because we don't want services bootstrapping their own duplicate inner services if we don't need to in tests. Now, forklift is able to take in an active audit database context that's shared with the test author's audit database context. Well, the inner PgPool is shared anyway.

Known Issue

If you manipulate filters in the audit logs UI, specifically clicking "Clear Filters", you will notice a significant slowdown. This is not specific to this PR and is an issue in "app/web" that will be solved in a follow-up PR.

This commit ensures that we read from the audit database for the audit logs interface rather than directly from the NATS JetStream stream. This is a drop-in replacement that does not result in a change to the front end. The user experience, however, should be vastly improved with one caveat that we'll cover at the end. To accomplish this, we always read from the database in SDF and in integration tests, but enabling the audit logs app for forklift will still be opt-in for production use cases. However, for CI integration tests, local integration tests and development builds with buck2 and cargo, forklift will have the audit logs app enabled by default. This commit removes the ability to read from the stream in the dal wholesale and also uses the new "AuditLogRow" struct and its methods to read from the database. We rely on the compound index for the "audit_logs" table in the audit database for quick querying. Context: This switchover was implemented for a variety of reasons, including but not limited to the following: - Data correctness in the frontend (e.g. the number of rows you ask for should now be what you get, or at least however many exist for your currently selected change set) - Performance - Rich query semantics - Data segregation by database Test related refactoring: Ever wondered why our services often have a "from_config" method that calls a "from_services" method? It's because we don't want services bootstrapping their own duplicate inner services if we don't need to in tests. Now, forklift is able to take in an active audit database context that's shared with the test author's audit database context. Well, the inner PgPool is shared anyway. Known issue: If you manipulate filters in the audit logs UI, specifically clicking "Clear Filters", you will notice a significant slowdown. This is not specific to this PR and is an issue in "app/web" that will be solved in a follow-up PR. Co-authored-by: Fletcher Nichol <[email protected]> Signed-off-by: Nick Gerace <[email protected]>

nickgerace · 2024-12-02T23:59:29Z

Cargo.lock

@@ -1927,6 +1927,7 @@ name = "dal-test"
 version = "0.1.0"
 dependencies = [
 "async-recursion",
+ "audit-logs",


I'm going to break up this crate in a future PR, starting with separating the database and stream functionalities.

nickgerace · 2024-12-02T23:59:49Z

lib/audit-logs/src/database.rs

+/// A row in the audit logs table of the audit database.
+#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
+pub struct AuditLogRow {


I don't love this struct name, but I'm waiting to refactor this once we know it works.

nickgerace · 2024-12-03T00:00:12Z

lib/audit-logs/src/database/config.rs

+/// The default [`PgPoolConfig`] used in the [`AuditDatabaseConfig`].
+pub fn default_pg_pool_config() -> PgPoolConfig {
+    PgPoolConfig {
+        dbname: DBNAME.into(),
+        application_name: APPLICATION_NAME.into(),
+        ..Default::default()
+    }
+}


Needed for TestContext assembly

nickgerace · 2024-12-03T00:00:44Z

lib/dal-test/src/lib.rs

 const ENV_VAR_PG_USER: &str = "SI_TEST_PG_USER";
 const ENV_VAR_PG_PORT: &str = "SI_TEST_PG_PORT";
 const ENV_VAR_KEEP_OLD_DBS: &str = "SI_TEST_KEEP_OLD_DBS";

+const ENV_VAR_LAYER_CACHE_PG_DBNAME: &str = "SI_TEST_LAYER_CACHE_PG_DBNAME";
+const ENV_VAR_AUDIT_PG_DBNAME: &str = "SI_TEST_AUDIT_PG_DBNAME";


@fnichol @johnrwatson we may need to populate this for CI

Nope, that all looks good, dal and sdf-server integration test ran okay!

nickgerace · 2024-12-03T00:01:44Z

lib/dal-test/src/lib.rs

+    let connection_metadata = Arc::new(nats.metadata().to_owned());
+    let jetstream_context = jetstream::new(nats);
+
+    let server = forklift_server::Server::from_services(


Use from_services pattern rather than from_config

nickgerace · 2024-12-03T00:03:21Z

lib/forklift-server/src/server.rs

+        jetstream_context: jetstream::Context,
+        instance_id: &str,
+        concurrency_limit: usize,
+        audit_bag: Option<(AuditDatabaseContext, usize)>,


This was a pain. Before, forklift constructed the context at the very possible last second, which was awesome for production use cases. However, if you already had a context with an in PgPool... oops. Now, we handle that.

nickgerace · 2024-12-03T00:04:52Z

lib/sdf-server/src/service/v2/audit_log/list_audit_logs.rs

+#[derive(Debug)]
+struct Assembler {
+    change_set_cache: HashMap<ChangeSetId, ChangeSet>,
+    user_cache: HashMap<UserPk, User>,
+}


This was ported from the prior implemented with the NATS JetStream stream. We may want the metadata that these caches provide for the frontend in the database in its own columns, but we decided to just extend the metadata on the way out to app/web for now.

cc @britmyerss @fnichol since we talked about this

nickgerace · 2024-12-03T00:05:24Z

lib/si-test-macros/src/expand.rs

+                #test_context.nats_conn().clone(),
+                #test_context.audit_database_context().to_owned(),


Just take the context already in use

fnichol · 2024-12-03T00:06:16Z

/try

fnichol · 2024-12-03T00:24:08Z

/try

Signed-off-by: Fletcher Nichol <[email protected]>

fnichol · 2024-12-03T17:42:02Z

/try

Signed-off-by: Fletcher Nichol <[email protected]>

fnichol · 2024-12-03T18:24:07Z

/try

nickgerace force-pushed the nick/3307a92 branch from a31ce72 to ca71157 Compare November 27, 2024 23:24

nickgerace force-pushed the nick/3307a92 branch from ca71157 to f1364cb Compare December 2, 2024 22:20

github-actions bot added the A-ci Area: CI configuration files and scripts label Dec 2, 2024

nickgerace force-pushed the nick/3307a92 branch 2 times, most recently from 45fbc9a to fdaf7a7 Compare December 2, 2024 22:45

nickgerace changed the title ~~Incomplete attempt at reading from audit database~~ Read from the audit database Dec 2, 2024

nickgerace force-pushed the nick/3307a92 branch 4 times, most recently from 926d1c4 to b274c8b Compare December 2, 2024 23:54

nickgerace marked this pull request as ready for review December 2, 2024 23:56

nickgerace force-pushed the nick/3307a92 branch from b274c8b to 08bddce Compare December 2, 2024 23:57

nickgerace force-pushed the nick/3307a92 branch from 08bddce to 0a012ed Compare December 2, 2024 23:58

nickgerace commented Dec 3, 2024

View reviewed changes

nickgerace requested a review from fnichol December 3, 2024 00:05

fnichol previously approved these changes Dec 3, 2024

View reviewed changes

fix(auth-api): lock version of prisma CLI to 5.x to fix build

f4fa541

Signed-off-by: Fletcher Nichol <[email protected]>

fnichol dismissed their stale review via f4fa541 December 3, 2024 17:41

github-actions bot added the A-auth label Dec 3, 2024

fix(dal-macros): regen test output to fix failing tests

cf38174

Signed-off-by: Fletcher Nichol <[email protected]>

sprutton1 approved these changes Dec 3, 2024

View reviewed changes

fnichol added this pull request to the merge queue Dec 3, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 3, 2024

Merged via the queue into main with commit accc159 Dec 3, 2024
49 checks passed

fnichol deleted the nick/3307a92 branch December 3, 2024 20:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read from the audit database #5038

Read from the audit database #5038

nickgerace commented Nov 27, 2024 •

edited

Loading

nickgerace Dec 2, 2024

nickgerace Dec 2, 2024

nickgerace Dec 3, 2024

nickgerace Dec 3, 2024

fnichol Dec 3, 2024

nickgerace Dec 3, 2024

nickgerace Dec 3, 2024

nickgerace Dec 3, 2024

nickgerace Dec 3, 2024

fnichol commented Dec 3, 2024

fnichol commented Dec 3, 2024

fnichol commented Dec 3, 2024

fnichol commented Dec 3, 2024

		#test_context.nats_conn().clone(),
		#test_context.audit_database_context().to_owned(),

Read from the audit database #5038

Read from the audit database #5038

Conversation

nickgerace commented Nov 27, 2024 • edited Loading

Description

Context

Test Related Refactoring

Known Issue

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fnichol commented Dec 3, 2024

fnichol commented Dec 3, 2024

fnichol commented Dec 3, 2024

fnichol commented Dec 3, 2024

nickgerace commented Nov 27, 2024 •

edited

Loading