Skip to content
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 ability to include metadata on every event #14

Merged
merged 15 commits into from
Dec 8, 2023
Merged

Add ability to include metadata on every event #14

merged 15 commits into from
Dec 8, 2023

Conversation

skylerkatz
Copy link
Collaborator

I find myself wanting to include some additional data to every event, even though the data is not entirely related to the event itself.

For example, in a Middleware I generate a unique request_id and I want that included in every event. I also want to include the current Team that the authenticated user is making the request in.

The implementation follows very closely to the way Laravel handles this in their Queue::createPayloadUsing() function.

I also added some docs, but not sure if this deserves its own page, vs somewhere else on an existing page.

skylerkatz and others added 2 commits November 18, 2023 12:48
@inxilpro
Copy link
Contributor

I love this, and definitely want something like this in Verbs. My one question is: do we want metadata to just be a plain array, or do we want to use a collection or bespoke Metadata class to provide some convenience methods?

@skylerkatz
Copy link
Collaborator Author

I figured a basic array was the easiest thing without overcomplicating it. A collection might be nice as well. If we did a full class, maybe it makes sense to put a second column on the event table for metadata and then we can use a custom eloquent cast.

@DanielCoulbourne
Copy link
Contributor

Just throwing some unstructured thoughts out there:

  • I think I favor a 2nd column for metadata
  • I think I prefer some knowable structure for metadata. Thus far almost everything you do in a handle() method is typed, so it's very hard to reference things that might not exist. I'm pretty into this as it makes me feel a lot safer replaying events if the classes are at least statically valid. Arrays scare me because I can envision adding keys to that array over time, getting out of sync with old events, and eventually getting to an un-replayable situation.
  • This plays into a broader conversation @joshhanley @inxilpro and I have been having sort of disjointedly about event versioning Event Versioning #21

@inxilpro
Copy link
Contributor

So I could see doing kinda the best of both worlds. We could introduce a Metadata class that looks something like:

class Metadata
{
  protected Collection $extra;
  
  public function __construct(array $data = [])
  {
    // 1. Populate any properties that exist using our serializer
    // 2. Push remaining data into $this->extra
  }
  
  public function __get(string $name)
  {
    return $this->extra->get($name);
  }
  
  public function __set(string $name, $value): void
  {
    $this->extra->put($name, $value);
  }
  
  public function __isset(string $name): bool
  {
    return $this->extra->has($name);
  }
}

And by default we would populate $event->metadata with an instance of that class. Then we could introduce a new configuration that's like verbs.metdata_class that lets you set a custom metadata subclass that has typed properties. Something like:

namespace App\Events\Support;

class Metadata extends \Verbs\Metadata
{
  public ?string $user_agent = null;
  
  public ?string $ip_address = null;
  
  public ?int $active_user_id = null;
}

And then in your config you'd set verbs.metdata_class to App\Events\Support\Metadata.

Once that's all hooked up, you could just typehint the Metadata class of your choice inside the event lifecycle methods.

public function handle(App\Events\Support\Metadata $metadata) {
  // Now I have my typed metadata object
}

And Verbs could handle the DI for you. But if you don't want/need a custom metadata object, you could just type hint the base class and it'd still work.

The upside of this is that a package could rely on metadata always being there without having to know its type ahead of time. A package could just dump values into the metadata payload, and if the end user wanted to access them with type safety, all they would have to do is add properties to their custom class.

@skylerkatz
Copy link
Collaborator Author

I really like that approach.

The EventStore::createMetadataUsing() function could look something like

EventStore::createMetadataUsing(function (string $event_class, App\Support\Metadata $metadata) {
    $metadata->request_id = (string) Str::uuid();

    return $metadata;
});

So you could still sprinkle EventStore::createMetadataUsing() throughout the codebase or third party packages.

@inxilpro
Copy link
Contributor

Yeah, that's cool. We could definitely support type-hinting either version in the createMetadataUsing callback, too.

What's your thinking for wanting the event class in the callback? I would think that anything that's event-specific would live in the event, right? I would think that any global callbacks should only apply global metadata…

@skylerkatz
Copy link
Collaborator Author

It is a bit of a holdover from the Queue implementation. I could still see it being useful if you had some event that shouldn't have metadata...like maybe your TeamCreated event cannot end up with metadata of a $teamId because you know there is no team when the callback is triggered, so you could just return if that event is passed in.

But it's a bit of a contrived example

@DanielCoulbourne
Copy link
Contributor

@skylerkatz @inxilpro how far from yes-yes-yes are you two on this feature?

I think I'm ok with the implementation you guys were discussing in the comments. Are we in agreement and just need to implement? Or is there a substantive disagreement still?

@inxilpro
Copy link
Contributor

I think we're pretty good. @skylerkatz — you want to take a stab at switching to a class-based approach? I think for the first version we could skip the custom DI stuff, and just know that we want to add that eventually.

@skylerkatz
Copy link
Collaborator Author

I think we are just on the "needs to implement stage". Which I plan on working on this week.

Signed-off-by: Skyler Katz <skylerkatz@hey.com>
Signed-off-by: Skyler Katz <skylerkatz@hey.com>
Signed-off-by: Skyler Katz <skylerkatz@hey.com>
Signed-off-by: Skyler Katz <skylerkatz@hey.com>
@skylerkatz
Copy link
Collaborator Author

@inxilpro I pushed up an initial implementation at a Metadata class, but I don't love it.

I think it may just be an organization thing, but would love your thoughts on it.

@inxilpro
Copy link
Contributor

inxilpro commented Dec 7, 2023

Somehow I missed this! Looking thru it now, my main question is why not store metadata on the event object? We can chat about this tomorrow maybe!

inxilpro and others added 4 commits December 7, 2023 15:28
A new class, MetadataManager, was added to manage the creation and manipulation of Metadata instances across different components. As part of this update, existing event and metadata handling code were refactored to utilize the new MetadataManager. The Metadata class now also implements the ArrayAccess interface, providing array-like access to its properties.

Co-Authored-By: Skyler Katz <skylerkatz@hey.com>
The metadata creation documentation has been updated to illustrate a change in the createMetadataUsing method. The example now shows how to directly set the 'team_id' on the Metadata object. It also demonstrates the option of returning an array for automatic metadata merging.
Copy link
Contributor

@inxilpro inxilpro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good approach.

@inxilpro inxilpro merged commit 68324e1 into hirethunk:main Dec 8, 2023
17 checks passed
@skylerkatz skylerkatz deleted the feature/apply-metadata-to-event-payload branch December 9, 2023 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants