search

cloudevents / sdk-go

Go SDK for CloudEvents
https://cloudevents.github.io/sdk-go/
Apache License 2.0
836 stars 221 forks source link

Add WithCustomAttributes for PubSub #976

Closed AndreasBergmeier6176 closed 1 year ago

AndreasBergmeier6176 commented 1 year ago

Enables to pre-fill PubSub Message Attributes independent of CloudEvent spec. Thus this allows for setting arbitrary Message Attributes (given that they are not later overwritten by CloudEvent Attributes).

duglin commented 1 year ago

rebase needed

duglin commented 1 year ago

(from issue)

I think from a clarity sense I would prefer to always know the serialization beforehand. To this end it might make sense to also introduce different types, say:

* `Attributes`, current type which is specific to CloudEvents - thus in the PubSub case get a `ce-` prefix

* `ExternalAttributes`, new type which are attributes not covered by the CloudEvent spec - thus in the PubSub case get no implicit prefix.

Using HTTP as an example, I see this Option: https://github.com/cloudevents/sdk-go/blob/af1a06c50c614e4455e902b5e05d562bfc5fabd1/v2/protocol/http/options.go#L53 which (I believe) is a way to distinguish between a CE attribute and a Protocol attribute (http header). Meaning, use this option for HTTP headers and then SetAttribute for CE attributes. Then the user doesn't need to know about prefixes, that's a problem for the SDK to deal with.

So, yes I think we should introduce two types of attributes but I'm not sure we need two Options. Is PubSub different enough from HTTP that we couldn't just have your new Options deal with non-CE attributes and then then use SetAttribute for the CE ones?

Or, if you're suggesting that it would be nice to also have a way to define CE attributes for all Events flowing over this PubSub client (ie. "initial" CE attributes) then I think we should consider that for all protocols for consistency.

AndreasBergmeier6176 commented 1 year ago

Using HTTP as an example, I see this Option:

https://github.com/cloudevents/sdk-go/blob/af1a06c50c614e4455e902b5e05d562bfc5fabd1/v2/protocol/http/options.go#L53 which (I believe) is a way to distinguish between a CE attribute and a Protocol attribute (http header). Meaning, use this option for HTTP headers and then SetAttribute for CE attributes. Then the user doesn't need to know about prefixes, that's a problem for the SDK to deal with.

So, yes I think we should introduce two types of attributes but I'm not sure we need two Options. Is PubSub different enough from HTTP that we couldn't just have your new Options deal with non-CE attributes and then then use SetAttribute for the CE ones?

The way PubSub works is that the official client sends (Message publish) Requests via REST/GRPC to Google APIs. So e.g. the PubSub Message Attributes are then serialized into the HTTP/Protobuf body. I have seen people requesting for ways of modifying the actual REST/GRPC communication - and thus would be careful about reusing HTTP Options. Technically using PubSub may use HTTP and thus it might be confusing for HTTP Options to have a different effect. Also, looking at the WithHeader code, it has some very specific HTTP code.

EDIT Deleted a lot of my response because I think it is important to understand first, how the non-ce Attributes work for other protocols.

AndreasBergmeier6176 commented 1 year ago

Using HTTP as an example, I see this Option:

https://github.com/cloudevents/sdk-go/blob/af1a06c50c614e4455e902b5e05d562bfc5fabd1/v2/protocol/http/options.go#L53 which (I believe) is a way to distinguish between a CE attribute and a Protocol attribute (http header). Meaning, use this option for HTTP headers and then SetAttribute for CE attributes. Then the user doesn't need to know about prefixes, that's a problem for the SDK to deal with.

~Is this really the only way of modifying HTTP Headers? Thus you would have to recreate the whole Protocol if you want to set a Non-ce Header, that is unique for every message (e.g. TraceIds)?~ Just found WithCustomHeader, which means there already is a similar precedent for HTTP - so it seems it may be most consistent to add a WithCustomAttributes for PubSub.

Now updated the implementation to follow precedent of WithCustomHeader as close as possible.

duglin commented 1 year ago

I'll look over your latest change, but wanted to comment on one bit:

and thus would be careful about reusing HTTP Options.

I didn't mention HTTP to suggest we use HTTP's options, rather I was just looking at patterns from other protocols to see what we should learn, or use, for consistency.

AndreasBergmeier6176 commented 1 year ago

To simplify the code I now moved setting the Attributes to where the PubSub Message gets built.

AndreasBergmeier6176 commented 1 year ago

I'll look over your latest change, but wanted to comment on one bit:

and thus would be careful about reusing HTTP Options.

I didn't mention HTTP to suggest we use HTTP's options, rather I was just looking at patterns from other protocols to see what we should learn, or use, for consistency.

Ah, misunderstood you there. Anyway - now this PR should be pretty consistent with HTTP protocol.

duglin commented 1 year ago

thanks for your patience.

duglin commented 1 year ago

ping @embano1 and @lionelvillard for review

AndreasBergmeier6176 commented 1 year ago

We are working with a fork now: https://github.com/abergmeier/cloudevents-sdk-go

AndreasBergmeier6176 commented 1 year ago

@AndreasBergmeier6176 would you say there's any breaking/changed behavior for existing users or is the implementation (as I understand it) consistent with the current behavior unless a user uses the new attribute functions?

The latter. Note that no current test had to be modified.