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

[Feature] Support Cloud Events #2130

Open
iancooper opened this issue May 24, 2022 Discussed in #2067 · 12 comments
Open

[Feature] Support Cloud Events #2130

iancooper opened this issue May 24, 2022 Discussed in #2067 · 12 comments
Assignees

Comments

@iancooper
Copy link
Member

Discussed in #2067

Originally posted by IlSocio April 7, 2022
Hello everyone...
I just noticed that Brighter is forcing these two fields to be Guid, while when we use directly the transports ASB or Rabbit, it doesn't exists this limitation.

Would you accept a PR to use the more generic "string" type instead of Guid?

In case that the string is not provided, it could still fallback to the default value of Guid.NewGuid().ToString(), but, at least, we have the opportunity to provide any string value for the MessageId and CorrelationId, without any particular restriction.

Actions

For v10 we should move these identifiers to be a string type but default to providing a Guid to support the existing behavior. This may help with some issues around serialization where it can be difficult to determine if a field is a Guid without trying to parse it anyway. If we just treat it as a unique string this becomes easier

@iancooper iancooper self-assigned this May 24, 2022
@iancooper iancooper changed the title Message.Id and Message.CorrelationId should be a string not a UUID [Feature] Message.Id and Message.CorrelationId should be a string not a UUID Jul 6, 2022
@iancooper
Copy link
Member Author

Changed title from: Message.Id and Message.CorrelationId should be a string not a UUID as this is needed for CloudEvents

@iancooper iancooper changed the title [Feature] Message.Id and Message.CorrelationId should be a string not a UUID [Feature] Support Cloud Events Apr 19, 2023
@jeastham1993
Copy link

@iancooper can I support with this work at all?

@iancooper
Copy link
Member Author

Hi @jeastham1993 I want to check in with @preardon because the telemetry work has given us cloud events headers, but perhaps via a weird route. I can ping him on a back channel too. That way we can establish what the requirements are for this work.

@iancooper
Copy link
Member Author

@jeastham1993 @preardon

For v10 we should move these identifiers to be a string type but default to providing a Guid to support the existing behavior.

We should pick this up in the V10 pre-release window as well though.

@jeastham1993
Copy link

Thanks @iancooper. Just let me know on this thread if I can help in any way :)

@iancooper
Copy link
Member Author

@jeastham1993 I think @preardon will share his thoughts, and the we can figure out what the ask is.

@preardon
Copy link
Member

@jeastham1993 I am happy to get this in for V10, If my memory serves me correctly we were also going to look at allows Message ID to not be a Guid @iancooper ?

@iancooper
Copy link
Member Author

i think there are several things here:

  • For any given transport, support binary cloud events by including the values in the headers; however I think we are writing these anyway via Telemetry, so the question is how we want to rationalize between the telemetry code populating these and just making sure that all our transports write and read CE headers.
  • Changing our headers so that we can capture the information that cloud events needs.

We discussed making our message id a string, because in interop other messaging platforms were not necessarily using a UUID; it happens that would mean were more aligned with CE as well

@iancooper
Copy link
Member Author

@jeastham1993 @preardon

This is the requirement I think:

  • Design: What Brighter headers map to which Cloud Events headers; do any need to change type?
  • Design: Which Cloud Events headers are missing from Brighter headers; we need to add these.
  • Implementation: For each transport, we should use binary mode and when we map headers to the native header format, use the appropriate cloud events name for the header; when we map back we should look for cloud event headers.
  • Implementation: When we map back, we should look for our old header name if the Cloud Events name is not there; this will allow consumers to consume messages produced under the old format for a while. We can drop this eventually.
  • UpdateTelemetry no longer needs to use the bag to update the headers (@preardon check?).

@jeastham1993 are you volunteering to have a go at this?

@preardon
Copy link
Member

@iancooper they sounds good to me, I think we should also consider changeing our RequestIds to Strings (I would still use GUIDs to generate them but I feel we support more with them as string), this will flow down to our Message Ids ect.

@jeastham1993 if you are volunteering to do this I'm happy to see what time I can make to give you a hand

@iancooper
Copy link
Member Author

iancooper commented Feb 22, 2024

@preardon and @jeastham1993

Cloud Events does have a C# SDK: https://github.com/cloudevents/sdk-csharp

I don't think we want to derive our message from CloudEvent. I'm not necessarily a fan of backing properties with a dictionary of values, but it may help us understand types and header values. More usefully it does show us how to map to individual protocols. I don't think we want to loose our Option type, HeaderResult etc, but it might be useful to see if we their code when we understand what our support needs to offer.

@iancooper
Copy link
Member Author

@preardon and @jeastham1993 I am going to pick this one up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants