-
Notifications
You must be signed in to change notification settings - Fork 455
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ Add events for account and record update/delete/deactivation #2661
Conversation
thanks for writing these up! having something concrete is always great for getting brain flowing. I'd be pretty inclined to roll up the account events in to 1-2 event types. Eg, either a single "account status change", or two events, one aligning with the So either a single event:
or two:
For the record events, it feels like we could also merge them in to a single
In addition to profile records, we would probably have automod track:
|
made some adjustments according to your feedback. couple of questions though
|
9422d76
to
0f15836
Compare
for 1: seems good to track if a record event is an update or creation just for completeness? I could imagine us deciding we want to track creation events for some specific record types, like labeler declarations, which are relatively high-stakes and low-volume in the network. for 2: we'd want to track the status of accounts on independent PDS instances. it does get a bit "loopy" to double-track this in our ozone instance for accounts on our PDS instances, maybe we would skip generating those events? remember that we'll also be splitting out infra takedowns as a distinct event type from current takedowns, as some separate tech debt to clean up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall very much like this shape/direction!
} | ||
} | ||
}, | ||
"identityEvent": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for completeness, there is a concept of "tombstoning" an identity. this has been very rare in practice. in theory it is a permanent deletion, though technically it can be reverted in PLC within a time period (72hr), and can be reverted any time with did:web
.
it is sort of a corner-case, but would actually be a good thing to track in ozone because otherwise it would be very hard for mods to understand what had happened: a deleted identity makes it pretty hard to learn anything about an account from the live network.
|
Yeah, I think in other contexts we also have it as I think for identity and record states, we know more clearly that there are just the 3x operations on a record (create, update, delete), and that an identity is active or tombstoned (this is inherited from the W3C DID semantics). I would do just a boolean for identity tombstoned. For records, it could in theory be just a boolean and the |
Sounds like you're not super against it and I like the idea of just storing an Also added an optional |
for here are the I would also put the record event looks good! if we can get a quick set of eyes from @devinivy on this it would be great. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few small Qs and lexicon suggestions. Looks good!
"type": "string", | ||
"knownValues": ["create", "update", "delete"] | ||
}, | ||
"cid": { "type": "string" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"cid": { "type": "string" }, | |
"cid": { "type": "string", "format": "cid" }, |
"required": ["timestamp"], | ||
"properties": { | ||
"comment": { "type": "string" }, | ||
"handle": { "type": "string" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"handle": { "type": "string" }, | |
"handle": { "type": "string", "format": "handle" }, |
@@ -317,6 +320,25 @@ export class ModerationService { | |||
} | |||
} | |||
|
|||
if (isAccountEvent(event)) { | |||
meta.active = !!event.active |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If active
is missing do we want to interpret it as though the account went inactive? I wonder if we may want to omit it from meta
altogether in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the !!
is confusing here but this is a rerouted from #account
from the firehose and the active
flag should be always set https://github.com/bluesky-social/atproto/blob/main/lexicons/com/atproto/sync/subscribeRepos.json#L133
@@ -184,6 +184,25 @@ export class ModerationViews { | |||
eventView.event.remove = event.removedTags || [] | |||
} | |||
|
|||
if (event.action === 'tools.ozone.moderation.defs#accountEvent') { | |||
eventView.event.active = !!event.meta?.active |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar question here!
@@ -395,7 +395,7 @@ | |||
"accountEvent": { | |||
"type": "object", | |||
"description": "Logs account status related events on a repo subject. Normally captured by automod from the firehose and emitted to ozone for historical tracking.", | |||
"required": ["timestamp", "status"], | |||
"required": ["timestamp", "status", "active"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a required field means that old events missing this field will become invalid. Is that fine in this case (i.e. all pass events in practice already include the field)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh haha nevermind! I was reading diff from recent commits and forgot this event was brand new in the PR 🙃
* ✨ Store deleted/updated event data in subject_status * 🐛 Fix query for recordDeletedAt and recordUpdatedAt * ✨ Add tombstoned status * ✨ Move from record to hosting term * ✅ Add tests for hosting params * ✨ Update lexicons for hostingStatuses * ✅ Update snapshots * ✅ Update snapshots * ✅ Update snapshots * ✨ Adjust hosting statuses
This PR adds 4 new events to enable ozone to keep track of account deletion/deactivation and record update/delete. The record update event will primarily be used for profiles and lists for the time being.
Questions: