Add clarification on including additional context attributes#301
Conversation
Signed-off-by: cathyhongzhang <cathy.h.zhang@huawei.com>
|
That's a great consolidation of all the extensibility discussions. LGTM. |
cneijenhuis
left a comment
cneijenhuis
left a comment
There was a problem hiding this comment.
I like the additions this PR makes after the introduction! 👍
However, I think the introductory sentences are overlapping with the first sentences of the section Extension Attributes (2 paragraphs down). I have used quotes to visualize the overlap.
I am also wondering if this paragraph should be part of the Extension Attributes section. That wouldn't only solve the issue of having to duplicate the introduction, but I'd personally expect e.g. the advice on "duplicating" attributes in context and data in the section on extension attributes.
| Every CloudEvent conforming to this specification MUST include one or more | ||
| of the following context attributes. | ||
|
|
||
| It could also include additional attributes in the "context attributes" that |
There was a problem hiding this comment.
CloudEvent producers MAY include additional extension attributes within the
event
There was a problem hiding this comment.
I will remove this duplication in the "Extension Attributes". I think it is better to put into the generic "Context Attribute" so as not to restrict it to extensions
There was a problem hiding this comment.
@cathyhongzhang I think it might make more sense to put this into the other section (the extension) section since this is really about adding extensions.
There was a problem hiding this comment.
I am not sure if all future new context attributes belong to extensions. If some context attribute is used by a large number of use cases, then the WG might reach consensus to add it to the main spec.md directly. So I think putting this in the generic "context attributes" section, which includes both attributes endorsed by this WG (i.e. those explicitly defined in spec.md) and those that are not endorsed, is better. What do you think?
There was a problem hiding this comment.
Currently none is defined in the spec.md, so by default, they all belong to extensions.
There was a problem hiding this comment.
I was just focused on the readability of it. This paragraph is really talking about extensions, granted its focused on "identity" extensions, but still extensions so it just felt a little out of place to have it here instead of in the extension section. Plus it then, sort of, reads like we're talking about extensions in 2 places instead of one.
In the end its not a big deal, it just felt a little odd, but I can live with it if others are ok with it.
| of the following context attributes. | ||
|
|
||
| It could also include additional attributes in the "context attributes" that | ||
| might be used in ancillary actions related to the processing of the event. |
There was a problem hiding this comment.
that might be used for any purpose in the processing of the CloudEvent
There was a problem hiding this comment.
Will remove the duplication
| It could also include additional attributes in the "context attributes" that | ||
| might be used in ancillary actions related to the processing of the event. | ||
| For example, in many IoT and enterprise use cases, an event could be used in | ||
| a serverless application that performs actions across multiple types of events. |
There was a problem hiding this comment.
[...] such as identifying or correlating event sources.
There was a problem hiding this comment.
will remove the duplication
| identity attributes to the "context attributes" which the event consumers can | ||
| use to correlate this event with the other events. If such identity attributes | ||
| happen to be part of the event "data", it is still suggested that the event | ||
| producer add the identity attributes to the "context attributes" so that event |
There was a problem hiding this comment.
also adds may be clearer - the attribute shouldn't be removed from "data".
I'm wondering if a more generic version of this applies to all extension attributes - it is completely fine if an attribute is "duplicated" in data and context.
There was a problem hiding this comment.
Agree it is completely fine if an attribute is duplicated in data and context and this applies to all attributes including extension attributes. Will use "also add" to be clearer.
Signed-off-by: cathyhongzhang <cathy.h.zhang@huawei.com>
| event. This enables event producers, or middleware, to include additional | ||
| metadata that might be used for any purpose in the processing of the | ||
| CloudEvent, such as identifying or correlating event sources. See | ||
| [CloudEvent Attributes Extensions](primer.md#cloudevent-attribute-extensions) |
There was a problem hiding this comment.
I think the last sentence, including this link, should remain.
There was a problem hiding this comment.
Will add it back to the extension section.
Signed-off-by: cathyhongzhang <cathy.h.zhang@huawei.com>
| both appear within the same JSON object. | ||
|
|
||
| ### Extension Attributes | ||
| CloudEvent producers MAY include additional extension attributes within the |
There was a problem hiding this comment.
I think this first sentence (in particular the "MAY") is important. I'm not sure why this section is being reduced.
There was a problem hiding this comment.
It is removed per comment on duplication with what's newly added in previous section
There was a problem hiding this comment.
I agree with removing duplicate text but not the normative statements behind them. I would suggest you try to move your new text into this section since they're both about extensions. Then we don't lose any of the normative language but add the clarifications you're looking for.
There was a problem hiding this comment.
The normative statements added in the previous generic "context attributes" section applies to extension attributes too since extension attributes are also "context attributes". Putting it into "extension" section adds implicit restriction/assumption that any future attribute will be an extension, which is not right. We can not predict the future and should allow the possibility of some future attribute to be added as standard attribute rather than extension attribute.
There was a problem hiding this comment.
@cathyhongzhang there aren't any normative statements in your new text though. By "normative" I mean using RFC2119 keywords.
Putting it into "extension" section adds implicit restriction/assumption that any future attribute will be an extension...
no it doesn't. Any new attribute added to the spec will not be an extension - it won't be since it's spec-defined. All of the attributes you're talking about in your new text are "extensions" because they are not defined by the spec, which is why it makes more sense to put the text into the "extensions" section. Its location in the spec does not change what identity properties people can add, it just make it more readable to put all extensions text into the same location.
There was a problem hiding this comment.
I agree with @duglin - and I'm sorry if my review wasn't clear on that. I prefer to merge your proposed text into the extension section.
Signed-off-by: cathyhongzhang <cathy.h.zhang@huawei.com>
|
LGTM |
of "Extension Attributes", and change "it is still suggested" to "SHOULD" Signed-off-by: cathyhongzhang <cathy.h.zhang@huawei.com>
Signed-off-by: cathyhongzhang <cathy.h.zhang@huawei.com>
|
I think this aligns with what we agreed to on today's call. Can I get one more LGTM from someone who was on today's call just as a quick double-check? Then I'll merge |
|
LGTM |
|
Approved on the 9/13 call with some minor edits - which are now done. |
5 participants
Signed-off-by: cathyhongzhang cathy.h.zhang@huawei.com