docs: Open edX events naming

[mariajgrimaldi]

Description:
This PR adds an Architectural Design Record for events naming, where it's defined how to identify them.

Dependencies: Before defining any other signal

Merge deadline: List merge deadline (if any)

Testing instructions:

Just documentation

Reviewers:

• @felipemontoya

Merge checklist:

• All reviewers approved
• CI build is green
• Version bumped
• Changelog record added
• Documentation updated (not only docstrings)
• Commits are squashed

Post merge:

• Create a tag
• Check new version is pushed to PyPI after tag-triggered build is
  finished.
• Delete working branch (if not needed anymore)

[ormsbee]

Really cool stuff! Left a couple of comments on the organization/ergonomics of this.

[ormsbee]

I think there's a really interesting fork in the road for how we want to think about versioning. OEP-41 was operating under the assumption that individual events are being versioned. This structure implies that the whole of the subdomain is versioned.

Honestly, at this point I wouldn't worry about encoding the version into package name. We want to put signals and their corresponding event data into the system in the most usable way for a Python developer. For instance, we could do something like this:

from openedx_events.learning.signals import STUDENT_REGISTRATION_COMPLETED

@receiver(STUDENT_REGISTRATION_COMPLETED)
def listen_for_registration(sender, registration_event_v1, **kwargs):
    # invoke celery task here.

In this scenario, we put all learning related signals in one module that probably looks something like:

# Just a long list of things that look like this, where we have some custom
# subclass of Django's Signal
STUDENT_REGISTRATION_COMPLETED = HookSignal(
    providing_args=['user_data', 'registration_data']
)

That way it's very easy to scan one module for the event information that you want. Also, at the Python level, signals can be decoupled from the data you get sent. Even if we completely change the format of the data we send for STUDENT_REGISTRATION_COMPLETED, as long as it still means the same thing (is sent at the same time in the lifecycle), we could have it send both v1 and v2 data as separate args, allowing the listener to grab whichever version of the event data that it cares about.

We can also get a little fancier with our subclass of Signal (named HookSignal here) if we want to add more metadata, like:

STUDENT_REGISTRATION_COMPLETED = HookSignal(
    event_type="org.openedx.learning.student.registration.completed",
    providing_args={
        'user_data': UserData,
        'registration_data': RegistrationData,
        'registration_data_v2': RegistrationDataV2,
    }
)

All of these are just ideas I'm throwing out, and I haven't really thought through all the implications, particularly around versioning. Maybe OEP-41 should be modified to differentiate between events (un-versioned), and the data in those events (always explicitly versioned). But my main point is that I think we should transform the data to more naturally fit the needs of our developer audience here, and that we should treat the serialized wire format as something to transform into later.

[ormsbee]

It might actually make sense to always lean into the fact that we can send multiple args to the Signal at the same time. That way, when commonly used things like UserDataV2 get introduced, everything that used to get UserData can now also get UserDataV2, instead of having user information scattered in different way across a bunch of event data objects that may or may not get updated.

[mariajgrimaldi]

Thank you so much for your response! It guided us in the right direction 🙌

We updated the PR so it follows your suggestions (about metadata, how to use the name, versioning... just great stuff), now it seems easier to adopt and understand. What do you think? also, we thought your HookSignal idea was super cool! 😄 So we'll be adopting that as well when defining signals. We'll keep you posted!

[ormsbee]

I got some feedback on the edX side from @feanil and @edx-abolger, who advocated for having a stronger versioning story for being able to remove a message type / signal in the future. I'll take a stab at writing up what this might translate into in terms of code later this evening or tonight–I think it'll be close to what's above, with version information baked into the signal name, e.g. STUDENT_REGISTRATION_COMPLETED_V1

[nasthagiri]

Ah. I posted my comments before seeing this thread.

I also would like to encourage us to think through the versioning aspect of this some more. I am inclined to think that adding a version number in the event name itself may not be necessary, however, we will likely need it in the payload itself. I am imagining that Plugins will need to interoperate with different versions of the core.

For example, PluginA will need to work with both Lilac and Maple versions. If the Maple version added additional fields to the event, then PluginA could write explicit code to use those fields by checking the version.

[felipemontoya]

As it so often happens, after posting I kept thinking about it and I think my understanding got a bit deeper when reading oep-41 once again.

If we go all in OEP-41 style and use the complete event message as the argument to the signal then we would have:

STUDENT_REGISTRATION_COMPLETED_V1.send(
    id="b3b3981e-7544-11ea-b663-acde48001122",
    type="org.openedx.catalog.course.created.v1",
    specversion="1.0",
    time="2020-02-23T09:00:00Z",
    source="/openedx/discovery/web",
    sourcehost="edx.devstack.lms",
    minorversion=2,
    data={
        # message specific payload here
    }
)

And the minorversion applies perfectly.

There is no corresponding majorversion
because that information is encoded into the message type.

In this case the major version already exists in the _V1 suffix in the signal name. This would also cover the "base standard on the payload of the events" that Nimisha suggested below.

In this case I would wrap the id, type, specversion, time and even perhaps source and sourcehost as things that the base HookSignal class calculates on the fly so that writing the signal at edx-platform is not so verbose as what I just wrote.

[ormsbee]

Sorry for not replying earlier. It's been a bit hectic for me lately. But it's sounds like we're converging on a design, which is super-exciting!

I haven't had time to organize the following as well as I would like, so my apologies if this rambles a bit. I would very much like to continue this discussion next week, but this is all I can do for now.

Use Cases

These are the ones that are top of mind to me:

1. As an Open edX developer, I want to be able to add a new event.
2. As an Open edX developer, I want to add additional information to an existing event, in a backwards compatible way.
3. As an Open edX developer, I want to be able to deprecate and eventually remove an event.
4. As an extension developer (Django app in separate repo, running in-process with edx-platform), I want to make sure that my app doesn't break as both edx-platform and the openedx_events repos evolve.
5. As an extension developer, I want to support multiple named releases of Open edX at the same time.
6. As a service developer, I want to be able to listen for events using a message queue.

Design Goals

I believe the principle design goal is to make these integrations maintainable. Things should evolve naturally, in a backwards compatible manner. When things break, they should break obviously in CI. They should not require edx-platform to run tests. This aspect is more important than the flexibility or power of the interface (more on that in a bit).

Major and Minor Versioning

In the abstract, we have a few different kinds of versioning flying around:

1. The major version of an event type. We increment this very rarely, when making backwards incompatible changes. This is a whole new signal, but we can start the naming convention without the version suffix–so go STUDENT_REGISTRATION_COMPLETED and then STUDENT_REGISTRATION_COMPLETED_V2 if it ever comes to it. I strongly suspect that we'll have fewer V2s than we think, and that it'll often get broken up into smaller, or differently named events.
2. The minor version of an event type. This is backwards compatible, and would get bumped periodically as new fields are added.
3. The pypi published version of the openedx_events library itself.
4. The named Open edX release.

Translating Version Considerations into Code

The built-in Django Signal class is relatively simple and permissive, but I think we can override the constructor and send methods to give us stronger guarantees (or for more flexibility, make a function to build one), and force errors to occur in CI. The providing_args parameter may be deprecated, and we don't have to pass it on to the superclass constructor, but we'll probably want richer data than that for ourselves. For instance, let's say we adopt the following policies:

• We stuff all the envelope stuff into a new field that callers don't have to specify. This is the auto-generated stuff, like ID, as well as stuff that could be derived from the instance of OpenEdxPublicSignal, like minorversion or type.
• We expect send calls to always send things over as keyword args, and throw an exception if we get any parameters that we don't expect, or any parameters that are missing, based on the constructor call to OpenEdxPublicSignal.
• Minor version bumps are always represented new fields that are added to the send call.

Catching Version Conflicts in Tests

# How things are laid out at event minor_version = 0
# openedx_events version 1.0

# openedx_events/learning/signals.py
STUDENT_REGISTRATION_COMPLETED = create_signal(
    event_type="org.openedx.learning.student.registration.completed",
    providing_args={
        'user': UserData,
        'registration': RegistrationData,
    },
)

# Some views.py
def post(self, request, format=None):
    # Do some work here
    STUDENT_REGISTRATION_COMPLETED.send(
        user=user_data,
        registration=registration_data
    )

# Caught in a signal handler in an extension
@receiver(STUDENT_REGISTRATION_COMPLETED)
def listen_for_registration(sender, user, registration, **kwargs):
    # invoke celery task here.

Then we decide to add demographics data to the signal, so the signal definition now looks like this:

# This is now minor_version=1
STUDENT_REGISTRATION_COMPLETED = create_signal(
    event_type="org.openedx.learning.student.registration.completed",
    providing_args={
        'user': UserData,
        'registration': RegistrationData,
        'demographics': DemographicsData,
    },
)

We've now added a new parameter for demographics. We didn't add it to user, (even though it is sort of user data). Making all additions into new parameters helps to make things more predictable.

At this point, our overridden send call makes this break:

# This throws an error, because we're sending a signal without filling in all
# the parameters. We don't want parts of edx-platform to emit signals with
# missing data.
STUDENT_REGISTRATION_COMPLETED.send(
    user=user_data,
    registration=registration_data
)

But this should still work, because the extra stuff is getting dumped into kwargs**:

# Still works, unchanged.
@receiver(STUDENT_REGISTRATION_COMPLETED)
def listen_for_registration(sender, user, registration, **kwargs):
    # invoke celery task here.

So that will force edx-platform to do the right thing and not break our extension when edx-platform starts sending the newer minor version of the signal. But what if we tried to access demographics data that wasn't being sent?

# Old views.py, doesn't send demographics
def post(self, request, format=None):
    # Do some work here
    STUDENT_REGISTRATION_COMPLETED.send(
        user=user_data,
        registration=registration_data
    )

# Newer extension, expects demographics
@receiver(STUDENT_REGISTRATION_COMPLETED)
def listen_for_registration(sender, user, demographics, **kwargs):
    # invoke celery task here.

This should break in the tests for the extension, because those tests will be written something like:

def test_receive_does_right_thing():
    # This will throw an error, because our listener wants a "demographics" arg,
    # as long as our send forces senders to use keyword only args, i.e. never
    # send positional ones. This means our class defines `send(**kwargs)`.
    #
    # Maybe this really isn't a subclass of Django Signals anymore, and it's
    # more like something that sits on top of Django Signals? Or maybe we ask
    # people to call a different method than send() (default our send to raise
    # an exception that tells people to call our special version, so we don't
    # have to override the signature).
    STUDENT_REGISTRATION_COMPLETED.send(
        user=user_data,
        registration=registration_data
    )
    assert did_the_thing()

So now we have a case where things are in sync. If the version expectations of edx-platform and the events library don't match up, it's an error that's hopefully caught in tests. Likewise if it doesn't line up for the extension. Both of them have a version of the events library that they specify in their requirements... but there's a wrinkle...

Decoupling Events Library from Event Versions

The problem is when we want to support multiple Open edX releases concurrently, where we want to lock the events to a specific version, but we want to continue to add features and bug fixes to the events library. Example features that would come in useful:

• Bugfix in JSON serialization.
• Improved logging.
• Adding Pulsar support.

But we can address this by splitting the library handling the logic for how to build and parse these events from the package defining the events.

So in this scenario, we could have a library that is nothing but a list of events (openedx_events) and a library that has the underlying logic/features, which I'm just going to call shaka for now (because it's late, and that's the first kind of signal that comes to mind). Shaka wouldn't be Open edX specific at all.

The openedx_events library would have shaka as a depenency, and let the version float for the most part. The only thing openedx_events imports from shaka is the create_signal function that it uses to populate the modules of signals it has. Then openedx_events has its own versioning, which will be {named_release}.{minor_revision}.{bug_fix}.

So after Lilac is cut, we say MAPLE = 1, and the version of openedx_events is 1.0.0. As events get added to, this goes to 1.1, 1.2, etc. When we cut Maple, it will get the latest Maple version of openedx_events, say 1.19. When trunk development happens for Nutmeg, openedx_events gets bumped to 2.0.

This would also be how we write extensions tests that support both versions simultaneously:

maple_only = pytest.mark.skipif(openedx_events.named_release == openedx_events.releases.MAPLE)

@maple_only
def test_fn():
    # Send event as expected in Maple.
    STUDENT_REGISTRATION_COMPLETED.send(
        user=user_data,
        registration=registration_data
    )

Or maybe we'd make a helper that sent an appropriate message depending on the version. Either way, the data necessary to do that would be there, and we'd be forcing people to actually test the signals in the way that edx-platform would be forced to send them. People would set up tox to force runs with multiple versions of the events.

Data

I think another implication of this is that the data being sent should be defined entirely in openedx_events itself, as attr objects (following the convention of OEP-49). There should be no models that can mutate over time and grow extra fields. No massive edx-platform dependency to import to test with. Nothing to circumvent the contracts we're creating. We basically want all JSON serializable data types, so Python primitives, datetimes, and such. The one broad exception we'd want to make are OpaqueKeys, and we'd probably teach the lib to be smart about serializing things like usage_key.

I think we'll also want to hide most of the CloudEvent envelope data for now, since I don't think it'll be relevant for most initial users who are doing this in-process. We will want to capture the information that represents class-level information that's associated with the event type, like:

• minor version
• event type

But in terms of sending that data to listeners, I think our long term solution should be to wrap it up in some kind of envelope parameter. But for now, I think we should just punt. None of those fields are directly relevant to listeners yet, and we can always start sending it as another kwarg to listeners later. We can be strict about what senders emit, while still allowing the framework itself to sneak in additional args in the future (like envelope, cloud_event_header, or whatever we end up calling this).

---

Okay, more to say on how versioning works across message queues, but I probably won't get time to write until next week. I don't think that's as important to hammer out right now though.

[felipemontoya]

As usual, there is a lot of unpack in every one of your answers.

Use Cases

I think the 6 cases you described cover everything we have discussed so far. I'd like to add that as extension developer I want to know when my extension will stop working before hand, which is the other side of the coin from the Case 3 "As an Open edX developer, I want to be able to deprecate and eventually remove an event". For this perhaps we need something similar to the RemovedInDjangoXXWarning notices.

Catching version conflicts in tests and decoupling the library

The patterns you propose for catching errors and conflicts in tests are very solid. What I don't see as strongly would be the need to decouple the library. I know this tailors the need of Case 5 about supporting multiple releases at the same time, but it also makes it way more complicated.

Currently when we make an open release we pin the dependencies and make security upgrades to django almost only. Eventually something else might get bumped for bug fixing but that is it. Decoupling the library would make it easy to pin the signal definition while continuing to work on the tooling behind. But we already don't upgrade those kinds of libraries in the supported release, let alone the previous unsupported ones.

I also don't see the need to bump major versions every time we make an open release. I think what we will see is more rapid development at the beginning when we add an initial set of signals and then relative stability of the library.

For example this would mean that Marple pinned v1.0 and then Nutmeg perhaps v1.7 (rapid evolution), but then Olive release also pinned 1.7 and only Palm pinned 1.8.
In this scenario as an extension developer I can still test the three scenarios by knowing which version was pinned at each release.

maple_only = pytest.mark.skipif(openedx_events.version != '1.0')
nutmeg_only = pytest.mark.skipif(openedx_events.version != '1.7')
...

@maple_only
def test_fn():
    # Send event as expected in Maple.
    STUDENT_REGISTRATION_COMPLETED.send(
        user=user_data,
        registration=registration_data
    )

@nutmeg_only
def test_fn():
    # Send event as expected in Maple.
    STUDENT_REGISTRATION_COMPLETED.send(
        user=user_data,
        registration=registration_data,
        demographics=demographics_data
    )

# ... and so for olive

Probably it will be the data end what is less stable.

Data

OEP-49 talks about a data.py file to house simple data objects that can be passed from your app’s function to the calling app.
If I understand your proposal correctly, then we should define similar data.py files in openedx_events and make OpenEdxPublicSignal fail on test if the arguments being passed to send don't conform (Started reading about python-attrs just now and it seems to be exactly for this).

They should not require edx-platform to run tests. This aspect is more important than the flexibility or power of the interface

This is what you mean, right? maintainability even if it costs the capacity to use in memory objects.

I suppose what will end up happening is that on the receiver end, developers will get the attr and in case they need the original object they will query the DB for it again. At least until stable python APIs are in place for working with this simple objects. If we come back to the use case 5, I believe developers who dare cross this line will be restricted by this primarily. More than they are by the signal definition.

I think even without the stable APIs in place yet, this approach is still preferable. Any developers listening for the events and not doing object calls will be free. Those that do can still use known patterns that allow proper testing.

While we are at it, should openedx_events define its own attr objects, or do we want to decouple them in a separate library? I would be fine with writing our own, but it is hard to get the right amount of DRY and not making a dependency hell.

[ormsbee]

Currently when we make an open release we pin the dependencies and make security upgrades to django almost only. Eventually something else might get bumped for bug fixing but that is it. Decoupling the library would make it easy to pin the signal definition while continuing to work on the tooling behind. But we already don't upgrade those kinds of libraries in the supported release, let alone the previous unsupported ones.

Good point. Maybe this is more a gut feeling on my part that these should be moving at separate cadences. I'm fine with keeping them together in the same repo for now, so long as they're separated well enough that it's easy to split them later if we start running into issues.

I also don't see the need to bump major versions every time we make an open release. I think what we will see is more rapid development at the beginning when we add an initial set of signals and then relative stability of the library.

You've had to deal with those version updates more than I have, so I won't push this one either. I suspect that we'll eventually end up in this situation anyway, because named release boundaries are the logical time to remove events we don't want to support any longer, and that'll be a major version bump in SemVer. So we cut Olive at 1.27, and after Olive is cut, we drop a few events and call the resulting package 2.0. To me, it's more intuitive that the package would follow our named releases, since so much of the use case is geared around those boundaries. But again, it's not something I feel strongly about, and it's easily changeable down the road, so I'm happy to drop it for now.

Probably it will be the data end what is less stable.

Yes. 100% this, which brings us to...

If I understand your proposal correctly, then we should define similar data.py files in openedx_events and make OpenEdxPublicSignal fail on test if the arguments being passed to send don't conform (Started reading about python-attrs just now and it seems to be exactly for this).

Yes, I'm proposing that there be a package that defines a bunch of attrs classes for things like UserData. I think the biggest value isn't necessarily doing type checking the parameters (though we can do that). The thing that's most important to me is that as long as edx-platform is emitting events that use only these data types defined in the openedx_events repo, then the extensions can be emitting the exact same thing in their tests. There's no need to import edx-platform and worry about a thousand dependencies. There's no need to mock out a piece of edx-platform and hope that the mock is close enough to reality and edx-platform won't make some incompatible change six months down the line.

They should not require edx-platform to run tests. This aspect is more important than the flexibility or power of the interface

This is what you mean, right? maintainability even if it costs the capacity to use in memory objects.

Yes, though I want to be careful about terminology. These are still in-memory objects, and we're not taking any network overhead (unless we're in an entirely different service, receiving over message queue, but none of that needs to be built out to start).

I suppose what will end up happening is that on the receiver end, developers will get the attr and in case they need the original object they will query the DB for it again. At least until stable python APIs are in place for working with this simple objects. If we come back to the use case 5, I believe developers who dare cross this line will be restricted by this primarily. More than they are by the signal definition.

Agreed. Ideally, the event parameters already has everything I need when I'm writing a signal receiver in my extension. If not, I can always import something from edx-platform and call a function there, but the moment I cross that line, I've given up the safety of this versioned contract that edx-platform and my extension are sharing.

I think that reducing the number of places where this sort of thing is necessary should be a major goal. Sometimes that might mean that the event gets a little more verbose, to accommodate common use cases. For instance, say that there's a COURSE_COMPLETED signal, and many receivers will want to send some kind of email to the user to congratulate them. Instead of sending the user_id and expecting the handler to call edx-platform functions for more information, we would send a whole UserData object, that would include the full name, email, etc. Maybe also an EnrollmentData object, with information about their enrollment type. This is consistent with OEP-49's recommendation to avoid callbacks.

While we are at it, should openedx_events define its own attr objects, or do we want to decouple them in a separate library? I would be fine with writing our own, but it is hard to get the right amount of DRY and not making a dependency hell.

I'd vote for openedx_events defining its own. The data structures are very much part of the shared contract, and it makes sense to me to go together in the same repo with the signals, as they'd often be worked on together. I haven't mapped it out, but I think it would be nice to have a set of primitives that are scoped to the whole subdomain–so each event is a composition of data structures that are also used by other signals, e.g. UserData, EnrollmentData, etc. (I think the Data suffix is useful to differentiate it from other classes like models, picked because attrs is like dataclasses). We just have to be careful to separate out by subdomain (e.g. openedx_events.learning.data vs. openedx_events.catalog.data), since even basic terms like "course" sometimes mean very different things depending on subdomain.

[ormsbee]

BTW, I realize that restricting the event data types to attrs classes with JSON-serializable attributes means that some events will not be a drop-in replacement for existing signals in edx-platform. I think that's okay. Most events are emitted in very few places, and I think it's okay to have those places emit multiple events instead. I think that it's also an opportunity for us to rationalize the naming conventions across event types, and also to break things up into smaller, more granular events, based partly on what its receivers are querying for when they receive the event.

For instance, one long term pet peeve of mine is that course_published is actually emitted in a bunch of places where it doesn't make intuitive sense–e.g. when libraries get saved, when drafts of courses get created, etc. It would be great to have course_published vs. draft_saved, along with a list of usage keys that have changed, etc. We can encourage new development to use the new signals, and leave the old one be for now.

[ormsbee]

FYI @feanil

[nasthagiri]

[seeking rationale] Can you provide additional context (perhaps in the Consequences section) on why this spec defers from OEP-41 in terms of not including the major version number in the event name?

[nasthagiri]

[seeking info] Will there be a subsequent ADR for a base standard on the payload of the events? For example, I'm looking to see whether we can include similar (but not necessarily all) fields to the ones listed in OEP-41, including id, time, and version.

[felipemontoya]

@ormsbee @feanil @nasthagiri

I updated the ADR to match the latest feedback and also split the decisions in 2 ADRs one for naming and versioning and one for payload data.

I am focusing on events for now and I'm sure the filters library will make mostly the same decisions. The wrinkle there is the passing of attr objects for filters. However we can discuss that once we get to the filters portion.

Fyi @stvstnfrd in case you want to follow up on yesterdays discussion.

[feanil]

Added one more suggestion that you can take or leave.

[felipemontoya]

After accepting the last round of feedback we are ready to merge