Improve Core Events to support additional metadata #829
Conversation
… create event from an entity
| This argument is always given in case of an UPDATE. | ||
| **kwargs (dict[str, any]): Any extra information that would be passed to the metadata event. | ||
| Note: you should pass only simple types: str, float, double as values.""" | ||
| raise Exception(f"Unexpected entity type: {type(entity)}") |
There was a problem hiding this comment.
Notes to reviewers:
This function must be overloaded for each entity supporting event creation. This one would be called as a fallback if the interpreter does not find any for a entity type.
☂️ Python Cov
Overall Coverage
New FilesNo new covered files... Modified Files
|
| while self._in_context_attributes_changed_collector: | ||
| _publish_event(*self._in_context_attributes_changed_collector.pop(0)) | ||
| for event in self._in_context_attributes_changed_collector: | ||
| Notifier.publish(event) |
There was a problem hiding this comment.
Note to reviewers: Adding more arguments to the Event class make the use of a arg list a bit cumbersome and very error prone (think the order being significant!).
That is why I propose to replace this list of args by a list of Event objects. Then, the logic does not change, except that it makes more sense to call Notifier.publish directly now...
| @@ -58,6 +60,7 @@ class EventEntityType(_ReprEnum): | |||
| } | |||
|
|
|||
|
|
|||
| @dataclass(frozen=True) | |||
There was a problem hiding this comment.
Note to reviewers: the use of dataclass makes the definition of this (read-only) class sleaker. It also provides for free default repr and hash.
| @@ -19,11 +19,38 @@ | |||
|
|
|||
| def _publish_event( | |||
| entity_type: EventEntityType, | |||
There was a problem hiding this comment.
note to reviewers: the only two mandatory arguments are the entity_type and the operation. I propose to move them at first position, and be the only positional arguments for better readability. All the others can be specified using the named argument notation (including potential extra medatata).
src/taipy/core/job/job.py
Outdated
| return Event( | ||
| entity_type=EventEntityType.JOB, | ||
| entity_id=job.id, | ||
| config_id=job._task.config_id, # OK |
There was a problem hiding this comment.
The previous comment may have an impact on this line.
src/taipy/core/job/job.py
Outdated
| return Event( | ||
| entity_type=EventEntityType.JOB, | ||
| entity_id=job.id, | ||
| config_id=job._task.config_id, # OK |
src/taipy/core/notification/event.py
Outdated
|
|
||
|
|
||
| @singledispatch | ||
| def make_event( |
There was a problem hiding this comment.
We should rename the method _make_event for now. Once we want to open this as an API for users, we can name it without the _ prefix, but this will require a lot of documentation, and compatibility with future versions. I believe we are not ready for that.
There was a problem hiding this comment.
Ok, but notice that Notifier.publish(event) is public and documented...
There was a problem hiding this comment.
The method has been renamed into _make_event.
gmarabout
changed the title
gmarabout
changed the title
There was a problem hiding this comment.
Looks really good to me!!! also learnt something new too! Thanks @gmarabout !
Goals
We would like to add additional data to core events so we can use it for telemetry.
The data we want to add are: the
config_idof the entity (will eventually be sent as an attribute), the possibility to add extra data, especially for job events (like duration/latency).What the PR solves
We will need to create events with different (meta)data depending on the entity type.
The problem is that the method
_publish_eventtakes a bunch of data that the callers must provide in a non-generic way. It means that if we add one field to an event, then we have to change it everywhere_publish_eventis used.Moreover, the method being static, it is difficult for the dynamic
_Reloaderand_Propertiesclasses to have a generic code.The biggest change in this PR is the attempt to make a central piece where the logic of creating an event is delegated to a
_make_eventfunction which is "overridden" per entity type using the built-in Pythonsingledispatchmechanism.This function could eventually totally replace the static
_publish_event.For now, the
_publish_eventis still there for few cases.Changes
Eventis now a frozen dataclassEvent.attribute_valueto contain the value of a changed attributeEvent.metadata, a dict to contain any extra information we would like to transmit via the event (ex:task_config_idfor job events)./_make_eventto create an event for a particular entity type. The base function corresponds to the "default" case, and will raise an error if called. The overloaded functions are implemented within each entity Python file (injob.pyfor jobs, inscenario.pyfor scenarios, etc) and will implement the logic of creating an event from this specific entity. This method is intended to centralize event creation.