Instrument more audit log events #5151

britmyerss · 2024-12-18T05:27:59Z

This PR adds coverage to the following areas:

Function Authoring (Test, Execute, Attach for each function kind, Detach, Delete, Create, Updating Metadata )
Schema Variant Authoring (Create, Regenerate, Delete, Unlock,)
Contributing a module
Components (Creating/Deleting Connections, Attaching/Orphaning)
Views (Create, Update, Delete)

To do still include:

Handling code changes
Handling View Diagram Objects
Updating Schema Variant Metadata
Installing a Module
More granular Func Binding updates (depending on how we want to approach this, for ex: updating the arguments for an attribute function)

via SpongeBob SquarePants on GIPHY

nickgerace · 2024-12-18T16:10:00Z

lib/dal/src/func/binding.rs

+            AttributeFuncDestination::Prop(prop_id) => Prop::get_by_id(ctx, *prop_id).await?.name,
+            AttributeFuncDestination::OutputSocket(output_socket_id) => {
+                OutputSocket::get_by_id(ctx, *output_socket_id)
+                    .await?
+                    .name()
+                    .to_string()
+            }
+            AttributeFuncDestination::InputSocket(input_socket_id) => {


nit for the future: I like to use Self when possible for readability and suppleness for refactoring. So the first one would be Self::Prop instead of AttributeFuncDestination::Prop.

nickgerace · 2024-12-18T16:11:15Z

lib/sdf-server/src/service/diagram/create_connection.rs

+            to_socket_id: request.to_socket_id,
+            to_socket_name: to_socket_name.clone(),
+        },
+        format!("{0}-{1}", to_component_name, to_socket_name),


nit for the future: I'm a bit surprised the linter didn't catch this, but you can do the following:

format!("{to_component_name}-{to_socket_name}"),

You know, I didn't realize you could use positional numbers like 0 and 1. Before format! was able to use {variable} style syntax, the best you could do was format!("{}-{}", to_component_name, to_socket_name). Anyway, @nickgerace's idea is the most current Rustic way, agreed!

@fnichol year the positional numbers are interesting! I've used them in the past, moreso when I want to display something multiple times. They're cool!

I will update this too!

nickgerace · 2024-12-18T16:16:56Z

lib/sdf-server/src/service/v2/func/binding/attribute/reset_attribute_binding.rs

+            EventualParent::Component(component_id) => {
+                let component = Component::get_by_id(&ctx, component_id).await?;
+                ctx.write_audit_log(
+                    AuditLogKind::DetachFunc {
+                        func_id,
+                        func_display_name: func.display_name.clone(),
+                        schema_variant_id: None,
+                        component_id: Some(component_id),
+                        subject_name: component.name(&ctx).await?.to_owned(),
+                    },
+                    func.name.clone(),
+                )
                .await?;
+            }


Great use of breaking out an if let into a match... sometimes you'll see uses of match when unnecessary to expose all the variants left unused to avoid bugs if more variants are added to the enum. Example:

enum MyEnum { One Two Three } impl MyEnum { pub fn name(&self) -> &'static str { match self { One => "one" Two => "one" Three => "one" } } }

By using a match, we cover all the variants by default unless we use a wildcard, variable or match guard.

If that function instead used if let One = self { // code } else { // other code }, then adding new variants could cause undesired behavior in the else block.

I bring this all up since this is a great example of a sneaky if let that might not have been obvious from static code analysis, but you found it for this PR. Nice!

nickgerace · 2024-12-18T16:19:26Z

lib/sdf-server/src/service/v2/func/binding/create_binding.rs

+                            let func = Func::get_by_id_or_error(&ctx, func_id).await?;
+                            ctx.write_audit_log(
+                                AuditLogKind::AttachManagementFunc {


Unrelated to your PR but this route is frightening with all the nested SHEESH! We should refactor this later...

Yeah, definitely worth a pass.

nickgerace · 2024-12-18T16:22:35Z

lib/sdf-server/src/service/v2/func/create_func.rs

+                ctx.write_audit_log(
+                    AuditLogKind::CreateFunc {
+                        func_display_name: func.display_name.clone(),
+                        func_kind: func.kind.into(),
+                    },
+                    func.name.clone(),
+                )
+                .await?;
+                let schema_variant_name =
+                    SchemaVariant::get_by_id_or_error(&ctx, schema_variant_id)
+                        .await?
+                        .display_name()
+                        .to_string();
+                ctx.write_audit_log(
+                    AuditLogKind::AttachCodeGenFunc {
+                        func_id: func.id,
+                        func_display_name: func.display_name.clone(),
+                        schema_variant_id: Some(schema_variant_id),
+                        component_id: None,
+                        subject_name: schema_variant_name,
+                    },
+                    func.name.clone(),
+                )


Right now we create audit logs outside of the dal, but I'm wondering if CreateFunc should just move to Func::new. Then again, some of the new methods are run by other copy or clone or duplicate methods... so the audit log doesn't always reveal the user intent...

Just rambling on your PR, but I'm thinking about making this less manual in some way. Feels like there's room for a crate in between the dal low level and the route authoring that could exist.

yeah I had similar thoughts - it's also notable we usually end up with 3 things per route, ws event(s), posthog events, and now audit events and usually the posthog and audit events are quite similar. Not sure what to do with it exactly, but it crossed my mind as I was in here.

Yep... not this PR but it's starting to bubble over.

nickgerace · 2024-12-18T16:26:24Z

lib/sdf-server/src/service/v2/func/create_unlocked_copy.rs

+    let variant = if let Some(schema_variant_id) = request.schema_variant_id {
+        SchemaVariant::get_by_id(&ctx, schema_variant_id).await?
+    } else {
+        None
+    };


Aren't all funcs attached to assets these days? How is this possible? (unrelated to your PR, just curious if you know)

Funcs can technically be attached to only components, it's in a bit of a half state though as we don't expose it in the UI today.

nickgerace · 2024-12-18T16:27:25Z

lib/si-events-rs/src/audit_log/v1.rs

+        // strings for now I suppose
+        kind: String,


oh - this was from we discussed adding types to si-events that are only for audit events. I had this in there before that convo and never came back. I will update!

Ah gotcha. I remember this. Sounds good!

nickgerace · 2024-12-18T16:28:22Z

lib/si-events-rs/src/audit_log/v1.rs

+        new_color: String,
+        old_component_type: String,
+        new_component_type: String,
+        //todo: what to do about the code?


What is this referring to?

Just calling out that for updating the code for a schema variant definition, I wasn't sure how to handle that and punted :)

fnichol

Not a small amount of change! Good news is that most of it is net-new.

fnichol

All right!

fnichol · 2024-12-18T21:38:54Z

lib/sdf-server/src/service/v2/module/contribute.rs

@@ -83,6 +93,7 @@ pub async fn contribute(
            "pkg_hash": response.latest_hash,
        }),
    );
+    ctx.commit().await?;


I'm guessing we want this now because ctx.write_audit_log().await? will flush its events on ctx.commit().await??

There was already a commit in the route! The bad GitHub diff got me too :/ took me a bit to find it

Side note: there may also be an equivalent to "publish immediately" for audit logs to skip the rebaser I think. I had it at some point... can't remember if it's still there.

britmyerss requested review from stack72 and nickgerace December 18, 2024 05:28

github-actions bot added A-sdf Area: Primary backend API service [Rust] A-dal labels Dec 18, 2024

nickgerace reviewed Dec 18, 2024

View reviewed changes

fnichol previously approved these changes Dec 18, 2024

View reviewed changes

Instrument more audit log events

cf01a33

britmyerss dismissed fnichol’s stale review via cf01a33 December 18, 2024 21:29

britmyerss force-pushed the brit/add-more-audit-events branch from c603164 to cf01a33 Compare December 18, 2024 21:29

britmyerss requested review from nickgerace and fnichol December 18, 2024 21:30

nickgerace approved these changes Dec 18, 2024

View reviewed changes

fnichol approved these changes Dec 18, 2024

View reviewed changes

britmyerss added this pull request to the merge queue Dec 18, 2024

Merged via the queue into main with commit b59baf6 Dec 18, 2024
8 checks passed

britmyerss deleted the brit/add-more-audit-events branch December 18, 2024 23:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Instrument more audit log events #5151

Instrument more audit log events #5151

britmyerss commented Dec 18, 2024

nickgerace Dec 18, 2024

nickgerace Dec 18, 2024

fnichol Dec 18, 2024

nickgerace Dec 18, 2024

britmyerss Dec 18, 2024

nickgerace Dec 18, 2024

nickgerace Dec 18, 2024

britmyerss Dec 18, 2024

nickgerace Dec 18, 2024

britmyerss Dec 18, 2024

nickgerace Dec 18, 2024

nickgerace Dec 18, 2024

britmyerss Dec 18, 2024

nickgerace Dec 18, 2024

britmyerss Dec 18, 2024

nickgerace Dec 18, 2024

nickgerace Dec 18, 2024

britmyerss Dec 18, 2024

nickgerace Dec 18, 2024

fnichol left a comment

fnichol left a comment

fnichol Dec 18, 2024

nickgerace Dec 18, 2024

nickgerace Dec 18, 2024 •

edited

Loading

Instrument more audit log events #5151

Instrument more audit log events #5151

Conversation

britmyerss commented Dec 18, 2024

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

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

Choose a reason for hiding this comment

fnichol left a comment

Choose a reason for hiding this comment

fnichol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickgerace Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

nickgerace Dec 18, 2024 •

edited

Loading