Add sampling extension. by inlined · Pull Request #243 · cloudevents/spec

inlined · 2018-06-20T20:53:40Z

This pull request supercedes #76 and builds upon #242.

Per agreement at the f2f meeting 2018-06-15, this will be ratified
as an extension but not currently as part of the core spec. The
early inclusion of the spec and the ability for side-cars to impose
sampling early in the pipeline make it reasonable to start with
this as an extension.

I know the original author hoped to make this a core feature, though
there are two upsides:

Due to other decisions on 2018-06-15, this will have zero impact
on JSON encoding (but will in Proto and may in SDKs)
Extensions now get a whole document. This lets us pitch sampling
in more detail.

Necessary additional changes:

Added Integer to our type system
Added convention for how scalar extensions should be documented.
I'm not really excited about this proposal so others are welcome.

Special thanks to @maplebed for kicking this effort off.

Closes #76

duglin · 2018-06-29T12:10:28Z

@inlined I'm confused as to why github doesn't see a conflict when this touches (and overrides) some of the same files from #242 - and in the process I think we'd lose some edits (like the rename of the section to Documented Extensions). Maybe a rebase will help - not sure.

duglin · 2018-06-29T12:16:53Z

+
+## Attributes
+
+### (self)


I wonder if using (self) is less clear than just putting the actual text here? I'd kind of prefer to just be explicit about it instead of having a level of indirection.

Changed extensions.md to say that structured extensions' documentation will conventionally have an attributes section and scalar extensionsdocumentation will conventionally have avalue` section.

duglin · 2018-06-29T12:17:04Z

+## Encoding
+
+### In-memory formats
+The Sampling extension uses the key `sampledRate` for in-memory formats


missing period?

Why do we mention this for this attribute but not others?

What do you mean? The distributed tracing extension doc also says

The Distributed Tracing extension uses the key distributedTracing for in-memory formats

Didn't realize it did, but I was thinking about the core properties. no biggie either way.

duglin · 2018-06-29T12:20:01Z

+  the number of similar events that happened but were not sent plus this event.
+  For example, if a system sees 30 occurrences and emits a single event, `rate`
+  would be 30 (29 not sent and 1 sent).
+  Consumers SHOULD assume a value of `1` when the extension is omitted.


s/SHOULD/MUST/ - although, its kind of weird to have any constraints when the extension isn't used.

Would it be better to say:
A value of 1 is the equivalent of this extension not being used at all.

I'm ambivalent. Taking the change.

duglin · 2018-06-29T12:21:36Z

+  would be 30 (29 not sent and 1 sent).
+  Consumers SHOULD assume a value of `1` when the extension is omitted.
+* Constraints
+  * The rate MUST be positive.


Perhaps "... MUST be greater than zero." ? Didn't want to get into the possible confusion around whether zero is positive or negative :-) so let's just be explicit.

I'll change the prose rather than lecture everyone that zero is neither positive nor negative =p

duglin · 2018-07-11T21:13:07Z

@inlined some comments need attention

@maplebed

This pull request supercedes cloudevents#76 and builds upon cloudevents#242. Per agreement at the f2f meeting 2018-06-15, this will be ratified as an extension but not currently as part of the core spec. The early inclusion of the spec and the ability for side-cars to impose sampling early in the pipeline make it reasonable to start with this as an extension. I know the original author hoped to make this a core feature, though there are two upsides: 1. Due to other decisions on 2018-06-15, this will have zero impact on JSON encoding (but will in Proto and may in SDKs) 2. Extensions now get a whole document. This lets us pitch sampling in more detail. Necessary additional changes: * Added `Integer` to our type system * Added convention for how scalar extensions should be documented. I'm not really excited about this proposal so others are welcome. Special thanks to @maplebed for kicking this effort off. Signed-off-by: Thomas Bouldin <inlined@google.com>

inlined

Added a bit more prose to the extensions.md to also warn about the danger of using custom transport-bindings. E.g. an MQTT -> HTTPS relay may drop custom transport bindings for extensions it is unaware.

inlined · 2018-07-25T17:34:44Z

+
+## Attributes
+
+### (self)


Changed extensions.md to say that structured extensions' documentation will conventionally have an attributes section and scalar extensionsdocumentation will conventionally have avalue` section.

inlined · 2018-07-25T17:36:34Z

+  the number of similar events that happened but were not sent plus this event.
+  For example, if a system sees 30 occurrences and emits a single event, `rate`
+  would be 30 (29 not sent and 1 sent).
+  Consumers SHOULD assume a value of `1` when the extension is omitted.


I'm ambivalent. Taking the change.

inlined · 2018-07-25T17:36:36Z

+  would be 30 (29 not sent and 1 sent).
+  Consumers SHOULD assume a value of `1` when the extension is omitted.
+* Constraints
+  * The rate MUST be positive.


I'll change the prose rather than lecture everyone that zero is neither positive nor negative =p

inlined · 2018-07-25T17:37:36Z

+## Encoding
+
+### In-memory formats
+The Sampling extension uses the key `sampledRate` for in-memory formats


What do you mean? The distributed tracing extension doc also says

The Distributed Tracing extension uses the key distributedTracing for in-memory formats

Signed-off-by: Thomas Bouldin <inlined@google.com>

duglin · 2018-07-25T18:10:34Z

+# Sampling extension
+
+There are many cases in an Event's life when a system (either the system
+creating the event or a system transporting the event) may wish to only emit a


s/may/might/

since this isn't a normative statement, its just exemplary

duglin · 2018-07-25T18:12:04Z

+There are many cases in an Event's life when a system (either the system
+creating the event or a system transporting the event) may wish to only emit a
+portion of the events that actually happened. In a high throughput system where
+creating the event is costly, a system may wish to only create an event for


s/may/might/ since this isn't a normative statement

duglin · 2018-07-25T18:12:21Z

+creating the event is costly, a system may wish to only create an event for
+1/100 of the times that something happened. Additionally, during the
+transmission of an event from the source to the eventual recipient, any step
+along the way may choose to only pass along a fraction of the events it


duglin · 2018-07-25T18:13:09Z

+
+In order for the system receiving the event to understand what is actually
+happening in the system that generated the event, information about how many
+similar events may have happened must be included in the event itself. This


s/may have happened must be/happened would need to be/

duglin · 2018-07-25T18:16:37Z

overall LGTM just a fit syntax things

Signed-off-by: Thomas Bouldin <inlined@google.com>

duglin · 2018-08-09T22:34:27Z

still LGTM

All - please review (and LGTM, or not) for next week's call

Vlaaaaaaad · 2018-08-10T07:19:32Z

LGTM

markpeek · 2018-08-10T15:58:26Z

+extension.

-## Documented Extensions
+As a convention, extensions of scalar types (`String`, `Binary`, `URI`,


This PR added an Integer type. Should either list it here or add "e.g." to not list all of the scaler types.

+1 to adding e.g.

Signed-off-by: Thomas Bouldin <inlined@google.com>

duglin · 2018-08-15T01:00:36Z

All, please review this one. I'm putting into the category of "easy/quick approval" for this week's call, so please speak up prior to the call if you see any issues.

duglin · 2018-08-16T18:42:48Z

Approved on 8/16 call

@maplebed

* Add sampling extension. This pull request supercedes cloudevents#76 and builds upon cloudevents#242. Per agreement at the f2f meeting 2018-06-15, this will be ratified as an extension but not currently as part of the core spec. The early inclusion of the spec and the ability for side-cars to impose sampling early in the pipeline make it reasonable to start with this as an extension. I know the original author hoped to make this a core feature, though there are two upsides: 1. Due to other decisions on 2018-06-15, this will have zero impact on JSON encoding (but will in Proto and may in SDKs) 2. Extensions now get a whole document. This lets us pitch sampling in more detail. Necessary additional changes: * Added `Integer` to our type system * Added convention for how scalar extensions should be documented. I'm not really excited about this proposal so others are welcome. Special thanks to @maplebed for kicking this effort off. Signed-off-by: Thomas Bouldin <inlined@google.com> * PR feedback; also add conttent to extensions.md Signed-off-by: Thomas Bouldin <inlined@google.com> * @douglin doesn't like the world 'may' 😉 Signed-off-by: Thomas Bouldin <inlined@google.com> * Add 'e.g.' to list of scalar types in conventions Signed-off-by: Thomas Bouldin <inlined@google.com>

maplebed mentioned this pull request Jun 21, 2018

Add sample rate field to the spec #76

Closed

duglin reviewed Jun 29, 2018

View reviewed changes

inlined force-pushed the extensions-breakout.sampling branch from 93b212e to d0d3e0b Compare July 25, 2018 17:40

inlined commented Jul 25, 2018

View reviewed changes

PR feedback; also add conttent to extensions.md

60ad09d

Signed-off-by: Thomas Bouldin <inlined@google.com>

inlined force-pushed the extensions-breakout.sampling branch from d0d3e0b to 60ad09d Compare July 25, 2018 17:42

duglin reviewed Jul 25, 2018

View reviewed changes

@douglin doesn't like the world 'may' 😉

c1c3ea4

Signed-off-by: Thomas Bouldin <inlined@google.com>

markpeek reviewed Aug 10, 2018

View reviewed changes

Add 'e.g.' to list of scalar types in conventions

9cdae21

Signed-off-by: Thomas Bouldin <inlined@google.com>

duglin merged commit 3248e31 into cloudevents:master Aug 16, 2018

duglin mentioned this pull request Nov 30, 2018

v0.2 #358

Merged

evankanderson mentioned this pull request May 2, 2019

Resolve quote issue for HTTP Binary Transport Encoding. #413

Closed

Conversation

inlined commented Jun 20, 2018 • edited by duglin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

duglin commented Jun 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duglin commented Jul 11, 2018

Uh oh!

inlined left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duglin commented Jul 25, 2018

Uh oh!

duglin commented Aug 9, 2018

Uh oh!

Vlaaaaaaad commented Aug 10, 2018

Uh oh!

markpeek Aug 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duglin commented Aug 15, 2018

Uh oh!

duglin commented Aug 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

inlined commented Jun 20, 2018 •

edited by duglin

Loading

markpeek Aug 10, 2018 •

edited

Loading