feat: Generate extension attributes from proto annotations #62

jskeet · 2021-06-09T15:02:55Z

No description provided.

amanda-tarafa

LGTM. This is nice.

The `cloud_event_extension_attribute` annotation describes the attribute within events.proto, including the name, description and optional camel-cased name to aid tooling. The `cloud_event_extension_name` annotation indicates that Eventarc will (sometimes conditionally) include the specified extension attribute for the given event. Purpose of this change: - Collect as much information about an event in these protos, as a single source of truth - Enable tooling to make these extension attributes easier for customers to find. See googleapis/google-cloudevents-dotnet#62 as an example PiperOrigin-RevId: 379281793

The `cloud_event_extension_attribute` annotation describes the attribute within events.proto, including the name, description and optional camel-cased name to aid tooling. The `cloud_event_extension_name` annotation indicates that Eventarc will (sometimes conditionally) include the specified extension attribute for the given event. Purpose of this change: - Collect as much information about an event in these protos, as a single source of truth - Enable tooling to make these extension attributes easier for customers to find. See googleapis/google-cloudevents-dotnet#62 as an example PiperOrigin-RevId: 379672582

* feat: Add proto annotations for extension attributes. The `cloud_event_extension_attribute` annotation describes the attribute within events.proto, including the name, description and optional camel-cased name to aid tooling. The `cloud_event_extension_name` annotation indicates that Eventarc will (sometimes conditionally) include the specified extension attribute for the given event. Purpose of this change: - Collect as much information about an event in these protos, as a single source of truth - Enable tooling to make these extension attributes easier for customers to find. See googleapis/google-cloudevents-dotnet#62 as an example PiperOrigin-RevId: 379672582 * docs: Remove TODO from Firestore event payload comment. PiperOrigin-RevId: 379709462 * Update to service catalog. PiperOrigin-RevId: 381051093 * Update to service catalog. PiperOrigin-RevId: 383638369 * docs: update readme catalog Signed-off-by: Grant Timmerman <timmerman+devrel@google.com> Co-authored-by: Copybara Bot <yoshi-code-bot@google.com> Co-authored-by: Jon Skeet <jonskeet@google.com> Co-authored-by: Grant Timmerman <744973+grant@users.noreply.github.com> Co-authored-by: Grant Timmerman <timmerman+devrel@google.com>

* feat: Add proto annotations for extension attributes. The `cloud_event_extension_attribute` annotation describes the attribute within events.proto, including the name, description and optional camel-cased name to aid tooling. The `cloud_event_extension_name` annotation indicates that Eventarc will (sometimes conditionally) include the specified extension attribute for the given event. Purpose of this change: - Collect as much information about an event in these protos, as a single source of truth - Enable tooling to make these extension attributes easier for customers to find. See googleapis/google-cloudevents-dotnet#62 as an example PiperOrigin-RevId: 379672582 * docs: Remove TODO from Firestore event payload comment. PiperOrigin-RevId: 379709462 * Update to service catalog. PiperOrigin-RevId: 381051093 * Update to service catalog. PiperOrigin-RevId: 383638369 * docs: update readme catalog Signed-off-by: Grant Timmerman <timmerman+devrel@google.com> * Update service catalogs PiperOrigin-RevId: 387121524 * Revert "Update service catalogs" This reverts commit 0266aac. This is to address the problem where 0266aac removes the CAL proto. This commit reverts that change. Later I will add a new CL that updates the service catalog. * Update to service catalog. PiperOrigin-RevId: 391849235 * Update Service Catalog on Sept 27, 2021 PiperOrigin-RevId: 399304046 Co-authored-by: Copybara Bot <yoshi-code-bot@google.com> Co-authored-by: Jon Skeet <jonskeet@google.com> Co-authored-by: Grant Timmerman <744973+grant@users.noreply.github.com> Co-authored-by: Grant Timmerman <timmerman+devrel@google.com> Co-authored-by: Jay Shi <jayshi@google.com> Co-authored-by: Averi Kitsch <akitsch@google.com>

* feat: Add proto annotations for extension attributes. The `cloud_event_extension_attribute` annotation describes the attribute within events.proto, including the name, description and optional camel-cased name to aid tooling. The `cloud_event_extension_name` annotation indicates that Eventarc will (sometimes conditionally) include the specified extension attribute for the given event. Purpose of this change: - Collect as much information about an event in these protos, as a single source of truth - Enable tooling to make these extension attributes easier for customers to find. See googleapis/google-cloudevents-dotnet#62 as an example PiperOrigin-RevId: 379672582 * docs: Remove TODO from Firestore event payload comment. PiperOrigin-RevId: 379709462 * Update to service catalog. PiperOrigin-RevId: 381051093 * Update to service catalog. PiperOrigin-RevId: 383638369 * docs: update readme catalog Signed-off-by: Grant Timmerman <timmerman+devrel@google.com> * Update service catalogs PiperOrigin-RevId: 387121524 * Revert "Update service catalogs" This reverts commit 0266aac. This is to address the problem where 0266aac removes the CAL proto. This commit reverts that change. Later I will add a new CL that updates the service catalog. * Update to service catalog. PiperOrigin-RevId: 391849235 * Update Service Catalog on Sept 27, 2021 PiperOrigin-RevId: 399304046 * Remove invalid method names from service catalog. PiperOrigin-RevId: 401831500 * Update Service Catalog PiperOrigin-RevId: 403148687 * Update Service Catalog on Oct 29, 2021 PiperOrigin-RevId: 406397501 * Update Service Catalog with missing methodName PiperOrigin-RevId: 406825642 * Add matrix_id field to CloudEvent payload for Firebase Test Lab PiperOrigin-RevId: 409185529 * Update Service Catalog on November 15, 2021 PiperOrigin-RevId: 409994579 Co-authored-by: Copybara Bot <yoshi-code-bot@google.com> Co-authored-by: Jon Skeet <jonskeet@google.com> Co-authored-by: Grant Timmerman <744973+grant@users.noreply.github.com> Co-authored-by: Grant Timmerman <timmerman+devrel@google.com> Co-authored-by: Averi Kitsch <akitsch@google.com>

averikitsch · 2022-01-27T19:19:01Z

@jskeet should this be merged or is this just a prototype example?

jskeet · 2023-04-20T07:52:24Z

It looks like cloud_event_extension_attribute is being fairly widely used, so I'll revisit this and hopefully be able to merge it.

… at generation time

This allows simpler access to proto annotations.

(This is the result of the previous commit.)

amanda-tarafa

LGTM, as my comments are for consideration. But this is fine as is as well.

amanda-tarafa · 2023-04-20T09:18:18Z

src/Google.Events.Protobuf.Tests/ExtensionAttributeTest.cs

+        var bytes = TestResourceHelper.LoadBytes("structured-mode-body-with-extension.json");
+        var formatter = new ProtobufJsonCloudEventFormatter<FunctionEventData>();
+        var evt = formatter.DecodeStructuredModeMessage(bytes, null, null);
+        Assert.Equal("sample-function", evt[ExtensionAttributes.Function]);


If Function is an extension that applies to another event, what's the error message?
Maybe a test for that, if only for documenting the error and trying to improve if needed, could be fine.

I know there's already a TODO on the generation code to take into account the event type the attributes apply to, which would help a lot here. I've left a comment there about that TODO maybe being something to do now?

Also, maybe consider making these actual extension methods: evt.GetFunction();? The Extension name predisposed me to finding extension methods.

If Function is an extension that applies to another event, what's the error message?

There won't be an error - it'll just return null (IIRC, anyway). I'll add a test for that, thanks.

I know there's already a TODO on the generation code to take into account the event type the attributes apply to, which would help a lot here. I've left a comment there about that TODO maybe being something to do now?

Hmm. I don't think it's worth changing how the attributes are generated (in terms of API surface)... partly because there's a problem here in terms of the same event message (e.g. FunctionEventData) being used for multiple event types.

However, I could:

Only generate them when we know of at least one event type

Generate documentation saying which event types they're used for

Also, maybe consider making these actual extension methods: evt.GetFunction();? The Extension name predisposed me to finding extension methods.

Right, that's CloudEvent terminology clashing with C# terminology. I don't think I want to add extension methods at the moment, partly because it's so easy to get into collisions if you use multiple namespaces. But we could add that later, if we get customer feedback.

Yep, all this sounds good, including documentation for the extension attributes. And thanks for the test.

amanda-tarafa · 2023-04-20T09:23:11Z

tools/Google.Events.Tools.CodeGenerator/Program.cs

@@ -125,6 +125,45 @@ private static void GenerateCode(IEnumerable<CloudEventDataInfo> infos, string s
            }
        }

+        private static void GenerateCloudEventAttributes(FileDescriptorSet descriptorSet, string srcDirectory, string project)
+        {
+            // TODO: Use the information about which extension attribute is used within which event type?


If you want to translate this to something static, say {EventType}ExtensionAttributes classes, that'd be a breaking change. Unless you are thinking more about validation?
I think that both static and validation would be nice.

I didn't have a specific scenario in mind - but I think documentation generation would be a good start, as described above.

Yep documentation sounds good actually.

This exposes extension attributes (as static properties) within a static ExtensionAttributes class per directory. The extension attributes are all expected to be strings, and are not currently used in the formatter - but they can still be accessed afterwards. (Test will come after generation commit.) Unfortunately, in order to implement this, the generator needed fairly significant rewriting.

This now generates the extension attributes

This uses a cut-down version of a real event - although Eventarc always delivers it in binary mode, which is harder to test, so I've converted it into structured mode.

jskeet · 2023-04-20T13:43:01Z

Okay, this is now rewritten to provide documentation about which event types are used for which extension attributes. PTAL @amanda-tarafa (when you have time).

amanda-tarafa

LGTM

amanda-tarafa · 2023-04-20T22:37:20Z

src/Google.Events.Protobuf/Cloud/ApiGateway/V1/ExtensionAttributes.g.cs

+        /// <summary>
+        /// <para>The resource ID of the Api triggering the event.</para>
+        ///
+        /// <para>This is used by the following events:</para>


amanda-tarafa · 2023-04-20T22:45:06Z

src/Google.Events.Protobuf/Cloud/Firestore/V1/Data.g.cs

@@ -69,8 +69,10 @@ public static partial class DataReflection {
            "KAsyNi5nb29nbGUuZXZlbnRzLmNsb3VkLmZpcmVzdG9yZS52MS5NYXBWYWx1",
            "ZS5GaWVsZHNFbnRyeRpWCgtGaWVsZHNFbnRyeRILCgNrZXkYASABKAkSNgoF",
            "dmFsdWUYAiABKAsyJy5nb29nbGUuZXZlbnRzLmNsb3VkLmZpcmVzdG9yZS52",
-            "MS5WYWx1ZToCOAFCLKoCKUdvb2dsZS5FdmVudHMuUHJvdG9idWYuQ2xvdWQu",
-            "RmlyZXN0b3JlLlYxYgZwcm90bzM="));
+            "MS5WYWx1ZToCOAFCdqoCKUdvb2dsZS5FdmVudHMuUHJvdG9idWYuQ2xvdWQu",


There are no extension attributes generated for Firestore and a few others, like CloudBuild and Storage. Just flagging in case that's not expected.

There are no extension attributes generated for Firestore and a few others, like CloudBuild and Storage. Just flagging in case that's not expected.

Just checked those three, and events.proto doesn't declare any. (I suspect they may be added later, as I'm pretty sure they should have them.) That won't be breaking, so I'm not too worried :)

jskeet requested a review from a team as a code owner June 9, 2021 15:02

product-auto-label bot added the api: eventarc Issues related to the googleapis/google-cloudevents-dotnet API. label Jun 9, 2021

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jun 9, 2021

jskeet mentioned this pull request Jun 9, 2021

Prototype of protobuf annotations to describe extension attributes googleapis/google-cloudevents#226

Closed

amanda-tarafa approved these changes Jun 9, 2021

View reviewed changes

jskeet force-pushed the attribute-annotations branch from 41a2dcb to 4a91ced Compare June 14, 2021 08:09

copybara-service bot mentioned this pull request Jun 14, 2021

feat: Add proto annotations for extension attributes. googleapis/google-cloudevents#227

Merged

rogerthatdev added the needs work This is a pull request that needs a little love. label Aug 29, 2022

jskeet added 3 commits April 20, 2023 09:07

chore: Allow an existing google-cloudevents directory to be specified…

5e1bce0

… at generation time

tools: Generate C# for cloudevent.proto

88229da

This allows simpler access to proto annotations.

chore: Generate C# code from cloudevent.proto

a7302cf

(This is the result of the previous commit.)

jskeet force-pushed the attribute-annotations branch from 3cb8fca to 3a91820 Compare April 20, 2023 08:54

jskeet requested a review from a team as a code owner April 20, 2023 08:54

jskeet changed the title ~~Prototype of generating extension attributes from proto annotations~~ feat: Generate extension attributes from proto annotations Apr 20, 2023

amanda-tarafa approved these changes Apr 20, 2023

View reviewed changes

jskeet force-pushed the attribute-annotations branch from 3a91820 to 3c0de53 Compare April 20, 2023 13:32

jskeet removed the needs work This is a pull request that needs a little love. label Apr 20, 2023

jskeet added 3 commits April 20, 2023 14:38

chore: Regenerate all code

04bb766

This now generates the extension attributes

tests: Test for generated extension attribute

7656d40

This uses a cut-down version of a real event - although Eventarc always delivers it in binary mode, which is harder to test, so I've converted it into structured mode.

chore: Remove unused using directives

bc6113d

jskeet force-pushed the attribute-annotations branch from 3c0de53 to bc6113d Compare April 20, 2023 13:39

amanda-tarafa approved these changes Apr 20, 2023

View reviewed changes

jskeet merged commit 50e03f8 into googleapis:main Apr 21, 2023
4 checks passed

jskeet deleted the attribute-annotations branch April 21, 2023 06:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Generate extension attributes from proto annotations #62

feat: Generate extension attributes from proto annotations #62

jskeet commented Jun 9, 2021

amanda-tarafa left a comment

averikitsch commented Jan 27, 2022

jskeet commented Apr 20, 2023

amanda-tarafa left a comment

amanda-tarafa Apr 20, 2023

jskeet Apr 20, 2023

amanda-tarafa Apr 20, 2023

amanda-tarafa Apr 20, 2023

jskeet Apr 20, 2023

amanda-tarafa Apr 20, 2023

jskeet commented Apr 20, 2023

amanda-tarafa left a comment

amanda-tarafa Apr 20, 2023

amanda-tarafa Apr 20, 2023

jskeet Apr 21, 2023

feat: Generate extension attributes from proto annotations #62

feat: Generate extension attributes from proto annotations #62

Conversation

jskeet commented Jun 9, 2021

amanda-tarafa left a comment

Choose a reason for hiding this comment

averikitsch commented Jan 27, 2022

jskeet commented Apr 20, 2023

amanda-tarafa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskeet commented Apr 20, 2023

amanda-tarafa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment