Update DG with use of callbacks & cloudEvents #56
Conversation
Add use of callbacks Add use of CloudEvents
bigludo7
requested review from
shilpa-padgaonkar,
rartych,
patrice-conil,
RubenBG7 and
jordonezlucena
as code owners
| | eventSubscriptionId | string | subscription identifier - could be valued for Resource-based subscription | optional | | ||
| | event | object | event structure - see next table | mandatory | | ||
| | id | string | identifier of this event, that must be unique in the source context. | mandatory | | ||
| | source | string - URI | identifies the context in which an event happened in the specific Provider Implementation. Often this will include information such as the type of the event source, the organization publishing the event or the process that produced the event. The exact syntax and semantics behind the data encoded in the URI is defined by the event producer. | mandatory | |
There was a problem hiding this comment.
According to CloudEvents specs
An absolute URI is RECOMMENDED
Should we provide more specific recommendations on syntax of source attribute?
There was a problem hiding this comment.
Have the same feeling, a suggestion aligned with the model proposed for type:
identifies the context in which an event happened in the specific Provider Implementation. Often this will include information such as the type of the event source, the organization publishing the event or the process that produced the event. The exact syntax and semantics behind the data encoded in the URI is defined by the event producer. For consistency accross CAMARA APIs we mandate following pattern: org.camara.api.name of the API (for example org.camara.api.device-status)
There was a problem hiding this comment.
Anyway, I have the doubt whether we should "change" "camara" wording by "$ob_name" as the source of the event is a Telco Operator. This would also apply for type
There was a problem hiding this comment.
Please discard my previous comment regarding the format, as it would not be compliant to CloudEvents specs.
As commented by Rafał, the recommendation is an absolute URI. If we are ok to go for this recommendation at least it needs to be indicated in the guidelines for any Operator to follow that format.
There was a problem hiding this comment.
According to CloudEvents specs
An absolute URI is RECOMMENDED
Should we provide more specific recommendations on syntax of source attribute?
Yes would be good to provide some recommendation or at least refer to examples provided here https://github.com/cloudevents/spec/blob/v1.0.2/cloudevents/spec.md#source-1
| | eventSubscriptionId | string | subscription identifier - could be valued for Resource-based subscription | optional | | ||
| | event | object | event structure - see next table | mandatory | | ||
| | id | string | identifier of this event, that must be unique in the source context. | mandatory | | ||
| | source | string - URI | identifies the context in which an event happened in the specific Provider Implementation. Often this will include information such as the type of the event source, the organization publishing the event or the process that produced the event. The exact syntax and semantics behind the data encoded in the URI is defined by the event producer. | mandatory | |
There was a problem hiding this comment.
Have the same feeling, a suggestion aligned with the model proposed for type:
identifies the context in which an event happened in the specific Provider Implementation. Often this will include information such as the type of the event source, the organization publishing the event or the process that produced the event. The exact syntax and semantics behind the data encoded in the URI is defined by the event producer. For consistency accross CAMARA APIs we mandate following pattern: org.camara.api.name of the API (for example org.camara.api.device-status)
| | eventSubscriptionId | string | subscription identifier - could be valued for Resource-based subscription | optional | | ||
| | event | object | event structure - see next table | mandatory | | ||
| | id | string | identifier of this event, that must be unique in the source context. | mandatory | | ||
| | source | string - URI | identifies the context in which an event happened in the specific Provider Implementation. Often this will include information such as the type of the event source, the organization publishing the event or the process that produced the event. The exact syntax and semantics behind the data encoded in the URI is defined by the event producer. | mandatory | |
There was a problem hiding this comment.
Anyway, I have the doubt whether we should "change" "camara" wording by "$ob_name" as the source of the event is a Telco Operator. This would also apply for type
| | datacontenttype | string | media-type that describes the event payload encoding, must be `application/json for CAMARA APIs` | optional | | ||
| | subject | string | describes the subject of the event. In CAMARA we enforce to use subject as defined in each API. This attribute is tagged as optional in CloudEvents specification but from CAMARA perspective we **strongly** recommend to value this attribute. | optional | | ||
| | time | string datetime| timestamp of when the occurrence happened (must adhere on CAMARA datetime recommendation based on RFC 3339) | optional | | ||
| | data | object| event notification details payload described in each CAMARA API and referenced by its `subject` | optional | |
There was a problem hiding this comment.
Think in CAMARA shoud be mandatory
There was a problem hiding this comment.
As of now you're probably right but I prefer to keep this flexible here in the guideline and let Working Group enforce data presence (and attributes) to each event type.
Added link to cloudevents.io Co-authored-by: Rafal Artych <121048129+rartych@users.noreply.github.com>
Co-authored-by: Rafal Artych <121048129+rartych@users.noreply.github.com>
Co-authored-by: Rafal Artych <121048129+rartych@users.noreply.github.com>
Reworked data description following Pedro Comment.
There was a problem hiding this comment.
Reworked data attribute description following @PedroDiez comment
Changed time & subject cardinality in Notification event schema
There was a problem hiding this comment.
Changed time & subject cardinality in Notification event schema following @rartych & @PedroDiez guidance.
|
Please provide any additional reviews or comments by 15th September |
There was a problem hiding this comment.
Following @shilpa-padgaonkar and @akoshunyadi :
Added version in the type attribute
Shifted endpoint from /event-notifications to /notifications
Thanks to @dfischer-tech review fixed some miscues.
|
Thanks @dfischer-tech @shilpa-padgaonkar and @akoshunyadi for your review ! @dfischer-tech As I worked on the notification part I have only fixed this part. Probably good to open an issue withe the identified adjustements. |
HI @murthygorty , I was highlighting that the term "instance-based subscription," as described in our documentation, actually the definition of an "Implicit subscription" rather than an "instance-based subscription." |
Co-authored-by: Jose Luis Urien <jlurien@gmail.com>
|
|
||
|
|
||
| Note on the operation path: | ||
| The recommended pattern is to use `/event-subscriptions` path for the subscription operation. But API design team, for specific case, has the option to append `/event-subscriptions` path with a prefix (e.g. `/roaming/event-subscriptions` and `/connectivity/event-subscriptions`). The rationale for using this alternate pattern should be explicitly provided (e.g. the notification source for each of the supported events may be completely different, in which case separating the implementations is beneficial). | ||
| The recommended pattern is to use `/subscriptions` path for the subscription operation. But API design team, for specific case, has the option to append `/subscriptions` path with a prefix (e.g. `/roaming/subscriptions` and `/connectivity/subscriptions`). The rationale for using this alternate pattern should be explicitly provided (e.g. the notification source for each of the supported events may be completely different, in which case separating the implementations is beneficial). |
There was a problem hiding this comment.
Wouldn't this kind of an (optional) categorization or extended information be more better in the type org.camaraproject.<optional-category or extension><api-name>.<api-version>.<event-name>rather than in the path? WDYT?
Other than that I am almost ready to approve this review :)
There was a problem hiding this comment.
@shilpa-padgaonkar Not sure to follow you. You propose to update the type attribute with an additional <optional-category or extension> but this point in the guideline is not about the event type but the /subscriptions endpoint.
What we say here is for explicit subscription pattern we recommend a generic /subscriptions endpoint but for some API with several capabilities we allow to 'specialize' this subscription endpoint (and provide several) to make it very explicit for the developer.
There was a problem hiding this comment.
@bigludo7 Then may be the examples were misleading to me :) Because the examples shown here at least to me seem as some sort of additional info /categorization about the event. If this is not the case, you can ignore my comment :)
There was a problem hiding this comment.
Thanks @shilpa-padgaonkar - indeed this is not related to the event directly but to the subscription.
| | GET | `/event-subscriptions/{eventSubscriptionsId}` | Operation to retrieve a event subscription | | ||
| | DELETE | `/event-subscriptions/{eventSubscriptionsId}` | Operation to delete a event subscription | | ||
| | POST | `/subscriptions` | Operation to request a event subscription. | | ||
| | GET | `/subscriptions` | Operation to retrieve a list of event subscriptions - could be an empty list. eg. `GET /subscriptions?type=ROAMING_STATUS&ExpireTime.lt=2023-03-17` | |
There was a problem hiding this comment.
The type field value here probably needs to align with the new convention?
There was a problem hiding this comment.
Agreed - will fix.
There was a problem hiding this comment.
On one side alignment of subscription type and notification type is good for uniformity, but on the other side only notifications are covered by CloudEvents specification and requirements and using the reverse DNS notation for event subscription looks strange for me.
Using old simple notation in subscription makes another feature more intuitive: within one subscription events of different types can be generated, for example: Subscription to type=ROAMING_STATUS can result in org.camaraproject.device-status.v1.roaming-on or org.camaraproject.device-status.v1.roaming-off events.
For clarity we can distinguish parameter names and have subscription kind and notification type as required by CloudEvents.
What are opinions of others?
There was a problem hiding this comment.
Hi @rartych,
I agree with you, using "kind" in subscription might be more user friendly than using event type.
Of course we need more opinions.
There was a problem hiding this comment.
@rartych - Regarding your comment - "Subscription to type=ROAMING_STATUS can result in org.camaraproject.device-status.v1.roaming-on or org.camaraproject.device-status.v1.roaming-off events." I think that doesnt seem right.
The modelling the Device Status API today seems to be as follows - when a subscription is made for "ROAMING_STATUS", the notification includes the "roaming" attribute as a boolean in the data payload. So you wouldn't receive two different "types" of notifications.
This is a different way to model (i.e., you still know on and off status but it isnt modelled as two different notification types).
I wonder if we could use that example as a general guideline in the API guidelines document for how to handle such a case (hope I made some sense). :)
There was a problem hiding this comment.
According to CloudEvents spec, the "type" value needs to be coherent among events and related subscriptions. Then, it is needed to have the same value and follow DNS-reverse format with the CAMARA pattern.
Reference:
https://github.com/cloudevents/spec/blob/main/subscriptions/spec.md#321-subscription-object
Type: Array of Strings - array of CloudEvents type values
Description: Indicates which types of events the subscriber is interested in receiving. When present on a
subscribe request, all events generated due to this subscription MUST have a CloudEvents type property that
matches one of these values.
...
| @@ -1183,16 +1198,16 @@ In order to ease developer adoption, the pattern for Resource-based event subscr | |||
|
|
|||
| | operation | path | description | | |||
| | ----- | ----- | ----- | | |||
| | POST | `/event-subscriptions` | Operation to request a event subscription. | | |||
| | GET | `/event-subscriptions` | Operation to retrieve a list of event subscriptions - could be an empty list. eg. `GET /event-subscriptions?type=ROAMING_STATUS&ExpireTime.lt=2023-03-17` | | |||
There was a problem hiding this comment.
Seems like "ExpireTime" isnt defined in the payload. It seems to be "expiresAt" (note also the case).
Also, do we have a vocabulary of operations such as "lt" defined somewhere? How would one know on which fields such operations are possible?
There was a problem hiding this comment.
Thanks - I will fix the expiresAt.
For the .lt this is defined in §8.3 filtering of this document.
|
One of the goals of the PR is "enforce use of callbacks in CAMARA API to describe notification event structure". Im not sure if that been covered. If it has, could you kindly point to the line numbers in the commit. Thank you in advance. |
Fixed 2 typos.
For me this covered in the 12.2 part. |
Renamed attribute (and aligned in the example) eventSubscription Id to subscriptionId to be aligned with endpoint name.
|
|
||
| **Instance-based subscription** | ||
| #### Instance-based (implicit) subscription |
There was a problem hiding this comment.
Are Instance-based subscriptions (explicit) and Resource-based subscriptions (implicit) outside the scope of our project or scenario doesn't exist?
There was a problem hiding this comment.
@sachinvodafone Sorry i did not understand your question - may I ask you to elaborate? Thanks
There was a problem hiding this comment.
Hi @bigludo7 , Just to clarify, I understand that an 'instance-based' subscription is associated with a specific event or instance of resource creation, whereas an 'implicit subscription' suggests that the subscription is established indirectly or automatically due to some other action or event.
Similarly, we also have two other distinct subscription types: 'resource-based' and 'explicit' subscriptions. In this context, we could potentially have four combinations:
- 'Instance-based subscriptions (explicit),'
- 'Instance-based subscriptions (implicit),'
- 'Resource-based subscriptions (explicit),'
- and 'Resource-based subscriptions (implicit).'
However, our documentation currently only covers two of these combinations:
- 'Instance-based subscription (implicit creation)'
- 'Resource-based subscription (explicit creation)'
There was a problem hiding this comment.
@sachinvodafone Got it ! thanks for the clarification
Instance-based and implicit are same thing, and resource-based and explicit are same; As suggested by @murthygorty I have introduced this vocabulary to explain the 2 patterns. I feel sorry it creates confusion. I could update it in next release if other people thinks that it adds more confusion than clarification.
There was a problem hiding this comment.
@sachinvodafone : If adding the above said description is not a blocking issue for you, could you please approve this PR as you were active in this review and kindly open a new issue for improving this documentation? Would that work for you?
|
@jlurien : Any blocking issues from your side? IF not, could you kindly approve, as you were an active reviewer? |
| A resource-based subscription is a event subscription managed as a resource. An API endpoint is provided to request subscription creation. As this event subscription is managed as an API resource, it is identified and operations to search, retrieve and delete it must be provided. | ||
| #### Resource-based (explicit) subscription | ||
|
|
||
| A resource-based subscription is a event subscription managed as a resource. This subscription is explicit. An API endpoint is provided to request subscription creation. As this event subscription is managed as an API resource, it is identified and operations to search, retrieve and delete it must be provided. |
There was a problem hiding this comment.
| A resource-based subscription is a event subscription managed as a resource. This subscription is explicit. An API endpoint is provided to request subscription creation. As this event subscription is managed as an API resource, it is identified and operations to search, retrieve and delete it must be provided. | |
| A resource-based subscription is an event subscription managed as a resource. This subscription is explicit. An API endpoint is provided to request subscription creation. As this event subscription is managed as an API resource, it is identified and operations to search, retrieve and delete it must be provided. |
| | DELETE | `/event-subscriptions/{eventSubscriptionsId}` | Operation to delete a event subscription | | ||
| | POST | `/subscriptions` | Operation to request a event subscription. | | ||
| | GET | `/subscriptions` | Operation to retrieve a list of event subscriptions - could be an empty list. eg. `GET /subscriptions?type=org.camaraproject.device-status.v1.roaming-status&expiresAt.lt=2023-03-17` | | ||
| | GET | `/subscriptions/{subscriptionsId}` | Operation to retrieve a event subscription | |
There was a problem hiding this comment.
| | GET | `/subscriptions/{subscriptionsId}` | Operation to retrieve a event subscription | | |
| | GET | `/subscriptions/{subscriptionId}` | Operation to retrieve a event subscription | |
| | POST | `/subscriptions` | Operation to request a event subscription. | | ||
| | GET | `/subscriptions` | Operation to retrieve a list of event subscriptions - could be an empty list. eg. `GET /subscriptions?type=org.camaraproject.device-status.v1.roaming-status&expiresAt.lt=2023-03-17` | | ||
| | GET | `/subscriptions/{subscriptionsId}` | Operation to retrieve a event subscription | | ||
| | DELETE | `/subscriptions/{subscriptionsId}` | Operation to delete a event subscription | |
There was a problem hiding this comment.
| | DELETE | `/subscriptions/{subscriptionsId}` | Operation to delete a event subscription | | |
| | DELETE | `/subscriptions/{subscriptionId}` | Operation to delete a event subscription | |
| @@ -1228,7 +1243,7 @@ Following Error code must be present: | |||
| * for `GET/{subscriptionId}`: 400, 401, 403, 404, 500, 503 | |||
| * for `DELETE`: 400, 401, 403, 404, 500, 503 | |||
There was a problem hiding this comment.
DELETE /{subscriptionId}
|
|
||
| Note: For operational and troubleshooting purposes it is relevant to accommodate use of `X-Correlator` header attribute. API listener implementations have to be ready to support and receive this data. | ||
|
|
||
| Specific eventType "SUBSCRIPTION_ENDS" is defined to inform listener about subscrition termination. It is used when the subscription expire time (required by the requester) has been reached or if the API server has to stop sending notification prematurely. For this specific event, the `eventDetail` must feature `terminationReason` attribute. | ||
| Specific event notification type "subscription-ends" is defined to inform listener about subscription termination. It is used when the subscription expire time (required by the requester) has been reached or if the API server has to stop sending notification prematurely. For this specific event, the `data` must feature `terminationReason` attribute. |
There was a problem hiding this comment.
| Specific event notification type "subscription-ends" is defined to inform listener about subscription termination. It is used when the subscription expire time (required by the requester) has been reached or if the API server has to stop sending notification prematurely. For this specific event, the `data` must feature `terminationReason` attribute. | |
| Specific event notification type "org.camaraproject.<api-name>.<api-version>.subscription-ends" is defined to inform listener about subscription termination. It is used when the subscription expire time (required by the requester) has been reached or if the API server has to stop sending notification prematurely. For this specific event, the `data` must feature `terminationReason` attribute. |
| | GET | `/event-subscriptions/{eventSubscriptionsId}` | Operation to retrieve a event subscription | | ||
| | DELETE | `/event-subscriptions/{eventSubscriptionsId}` | Operation to delete a event subscription | | ||
| | POST | `/subscriptions` | Operation to request a event subscription. | | ||
| | GET | `/subscriptions` | Operation to retrieve a list of event subscriptions - could be an empty list. eg. `GET /subscriptions?type=ROAMING_STATUS&ExpireTime.lt=2023-03-17` | |
There was a problem hiding this comment.
According to CloudEvents spec, the "type" value needs to be coherent among events and related subscriptions. Then, it is needed to have the same value and follow DNS-reverse format with the CAMARA pattern.
Reference:
https://github.com/cloudevents/spec/blob/main/subscriptions/spec.md#321-subscription-object
Type: Array of Strings - array of CloudEvents type values
Description: Indicates which types of events the subscriber is interested in receiving. When present on a
subscribe request, all events generated due to this subscription MUST have a CloudEvents type property that
matches one of these values.
...
|
As most active reviewers have approved the PR and confirmed that there are no more blocking issues, will merge this PR. Thank you all for the great collaboration on this PR and a big thanks to @bigludo7 for driving this topic . |
What type of PR is this?
What this PR does / why we need it:
In this PR several notifications update are provided:
callbacksin CAMARA API to describe notification event structureAdditionally this PR should be accompanied with PR #43.
This PR triggered cancellation of PR #41
Which issue(s) this PR fixes:
Fixes #8 #44
Note: Issue 44 covers several topics. One topic is about general ruling to have separate (or not) swagger for resource-based subscription from the nominal swagger for the API. This point is not tackled in this PR and must be followed in a new issue.
Special notes for reviewers:
Changelog input
Additional documentation
This section can be blank.