-
Notifications
You must be signed in to change notification settings - Fork 584
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
"Stand-alone event format" instead of "in-memory format" #538
Conversation
to "stand-alone event format" to better reflect the recent terminology changes in the spec. Signed-off-by: Klaus Deissner <klaus.deissner@sap.com>
@deissnerk pointed out to me that @alanconway introduced the phrase "stand-alone event format" in his most recent PR, so this PR is just aligning with that. The use of "in-memory format" in the docs is kind of confusing to me so I like this PR because it at least makes us consistent and will now use "stand-alone" in all of those places. However, I'm still not sure if people will know what a "stand-alone event format" means. We're just lucky that in most uses of it we also say "e.g. JSON" to help explain it. So, LGTM but I wouldn't object to another phrase that was more clear - perhaps one that linked to our use of the phrase "structured mode". |
@duglin - self-contained or self-describing event format kind of ring true for me, as does stand-alone |
"self-describing" is kind of an interesting term - w/o any more background info, that would mean more to me than "stand-alone". But, I do wonder if we need to have two terms "stand-alone" and "structured" - if feels like we should use one and stick with it - or at least formally link to the two, which I don't think we do now. |
I think we don't need to use "stand-alone" - I was trying to distinguish from the AMQP "format" spec, which is tied to the AMQP binding. Now we've merged that spec into the AMQP binding. I think the practical definition here is that: "an event format specifies how to serialize an event as a sequence of bytes and an "application/cloudevents+..." media-type describing the encoding. Any event format can be used to create a "structured" event message in any protocol binding that supports structured messages." |
Note: we could define a proper AMQP event format with an "application/cloudevents+amqp" media type and a small amount of extra direction on how to use the relevant sections of the Oasis AMQP framing rules to completely encode an event. My only concern with the original AMQP format is that it was not complete enough so that, e.g. an event could be serialized as AMQP bytes, embedded in a HTTP structured message and decoded again on the other end by an AMQP-aware CloudEvents endpoint. We could complete it, but I'm not sure that was ever the intent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hadn't looked at these specs before. I'm afraid that the "in memory format" sections don't make any sense to me, regardless of the term used.
documented-extensions.md
Outdated
Extensions always follow a common placement strategy for in-memory formats (e.g. | ||
[JSON](json-format.md), XML) that are decided by those | ||
Extensions always follow a common placement strategy for stand-alone event | ||
formats (e.g. [JSON](json-format.md), XML) that are decided by those | ||
representations. Protocol bindings (e.g. [HTTP](http-protocol-binding.md), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea what this means. If you have JSON or XML bytes in memory then obviously they follow the format rules for JSON or XML. If you have an API that decodes events to some generic Event object, then there's no format that "decides" the "placement" of anything. It is most likely that all extensions will be held in some kind of map or hash table, but that's well out of scope for this spec to determine, and definitely not related to any specific format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest we just replace this paragraph with:
Extensions attributes, while not defined by the core CloudEvents
specifications, MUST follow the same serialization rules as defined
by the format and protocol binding specifications. See
https://github.com/cloudevents/spec/blob/master/spec.md#extension-context-attributes
for more information.
While I do like the text about secondary serializations, we already cover
that in the main spec so we don't need to repeat it here, the ptr should be good
enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
||
The Distributed Tracing extension uses the key `distributedtracing` for | ||
in-memory formats |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is a "key"? Is this trying to mandate something about an API? There must be a field or method named "distributedtracing", or the string "distributedtracing" must be used as a map key? I don't understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if this was thinking there was a single bag/complex-type to hold all tracing attributes - if so, we don't allow that so I think we can drop this section and then things just default to traceparent/ce_traceparent per the core specs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may have its origin in when we allowed extensions to deviate from naming conventions to comply with existing contracts. As I understand it, this was supposed to express that the attribute name in event formats is "distributedtracing". As extensions now always have a defined attribute name and MAY have a secondary representation in addition, I suppose this can be dropped.
extensions/partitioning.md
Outdated
@@ -30,13 +29,12 @@ those events. | |||
|
|||
## Encoding | |||
|
|||
### In-memory formats | |||
### Stand-alone event formats | |||
|
|||
The partitionkey attribute extension uses the key `partitionkey` for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again: what is a "key"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would drop this section since the key name is the same as the attribute name and will just cause confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (we're on a roll!)
extensions/distributed-tracing.md
Outdated
@@ -26,10 +26,10 @@ Prometheus are built. | |||
|
|||
## Encoding | |||
|
|||
### In-memory formats | |||
### Stand-alone event formats |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is an "in memory format"? I can't find a definition.
extensions/sampled-rate.md
Outdated
@@ -30,9 +30,9 @@ they impose additional sampling. | |||
|
|||
## Encoding | |||
|
|||
### In-memory formats | |||
### Stand-alone event formats |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's just re-do this section to talk about what the 'attribute' is - like tracing does. Drop the notion of in-memory or stand-alone
LGTM!
…On Wed, Oct 16, 2019 at 10:33 AM Doug Davis ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In extensions/sampled-rate.md
<#538 (comment)>:
> @@ -30,9 +30,9 @@ they impose additional sampling.
## Encoding
-### In-memory formats
+### Stand-alone event formats
let's just re-do this section to talk about what the 'attribute' is - like
tracing does. Drop the notion of in-memory or stand-alone
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#538?email_source=notifications&email_token=AB3LUXQCNWQ7XOEMXWSI4Y3QO4Q2BA5CNFSM4JBCNBQ2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCIE265Y#pullrequestreview-302624631>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB3LUXUVZBCH3X4ED6EODWDQO4Q2BANCNFSM4JBCNBQQ>
.
|
If @alanconway likes it, I like it. At least at this stage, because otherwise we won't get done. ;) LGTM |
Signed-off-by: Klaus Deissner <klaus.deissner@sap.com>
Signed-off-by: Klaus Deissner <klaus.deissner@sap.com>
Signed-off-by: Klaus Deissner <klaus.deissner@sap.com>
Approved on the 10/17 call |
As we recently added a definition for event format that mentions stand-alone event formats to the spec, I changed the wording in the descriptions of extensions from in-memory format to "stand-alone event format" to better reflect this.
@alanconway What do think?