Forward ext.metadata (privTags) on the cross-platform serialization path#1472
Forward ext.metadata (privTags) on the cross-platform serialization path#1472bmehta001 wants to merge 6 commits into
Conversation
…n path
The UTC path maps EventInfo.PrivTags into ext.metadata, and JsonFormatter maps it
to ext.metadata.privTags, but the cross-platform bond (CsProtocol) serializer had
no ext.metadata extension, so on that path the field was only emitted as a Part C
property. Add a MetaData extension (privTags) to the CsProtocol schema and route
EventInfo.PrivTags into record.extMetadata in EventPropertiesDecorator, matching
the UTC/JSON behavior.
- CsProtocol.bond / CsProtocol_types.hpp: add MetaData { privTags } and
Record.extMetadata (field 38), defined unconditionally (next to M365a) so it is
present in the default build, not just HAVE_CS4_FULL.
- CsProtocol_readers.hpp / CsProtocol_writers.hpp: (de)serialize the new extension.
- EventPropertiesDecorator: route EventInfo.PrivTags into ext.metadata.privTags
instead of emitting it as a Part C property.
- tests: EventPropertiesDecoratorTests.Decorate_PrivTags_RoutedToExtMetadata.
NOTE: the ext.metadata extension ordinal (38) and field layout are best-effort and
must be confirmed against the canonical Common Schema wire contract before relying
on them; see the TODO(privacy-parity) markers.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds a missing Common Schema ext.metadata extension to the cross-platform Bond (CsProtocol) serialization path and routes EventInfo.PrivTags into ext.metadata.privTags for parity with other serialization paths.
Changes:
- Extend the
CsProtocolschema withMetaData { privTags }and addRecord.extMetadataat field ordinal 38. - Update generated Bond readers/writers to (de)serialize the new
extMetadatafield. - Update
EventPropertiesDecoratorto routeEventInfo.PrivTagsintorecord.extMetadata[0].privTags, and add a unit test asserting routing/no Part C duplication.
Reviewed changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unittests/EventPropertiesDecoratorTests.cpp | Adds a unit test ensuring PrivTags is routed to extMetadata and removed from Part C properties. |
| lib/include/public/CsProtocol_types.hpp | Introduces MetaData and adds Record.extMetadata (field 38) plus equality support. |
| lib/decorators/EventPropertiesDecorator.hpp | Routes EventInfo.PrivTags into ext.metadata.privTags instead of Part C emission. |
| lib/bond/generated/CsProtocol_writers.hpp | Adds Bond serialization for MetaData and Record.extMetadata (field 38). |
| lib/bond/generated/CsProtocol_readers.hpp | Adds Bond deserialization for MetaData and Record.extMetadata (field 38). |
| lib/bond/CsProtocol.bond | Updates the Bond schema to define MetaData and Record.extMetadata (ordinal 38). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Route the privacy tag (EventInfo.PrivTags) into ext.metadata.privTags so the | ||
| // cross-platform serialization path carries it the same way the UTC and JSON | ||
| // paths already do, rather than emitting it as a Part C property. | ||
| // TODO(privacy-parity): confirm the canonical Common Schema ext.metadata wire contract. | ||
| if (k == COMMONFIELDS_EVENT_PRIVTAGS) | ||
| { | ||
| if (record.extMetadata.empty()) | ||
| { | ||
| record.extMetadata.push_back(::CsProtocol::MetaData()); | ||
| } | ||
| record.extMetadata[0].privTags = static_cast<uint64_t>(v.as_int64); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Good catch - fixed in the latest push. The routing is now guarded on v.type == EventProperty::TYPE_INT64 (matching the precedent at lib/system/EventProperties.cpp:318), so a non-int64 EventInfo.PrivTags no longer reads the inactive union member - it falls through to normal Part C handling. Added a regression test (Decorate_PrivTags_NonInt64_FallsThroughToPartC).
| record.extMetadata[0].privTags = static_cast<uint64_t>(v.as_int64); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Right - addressed in the same push. Since the decorator now moves EventInfo.PrivTags out of Part C into record.extMetadata, I updated JsonFormatter::getJsonFormattedEvent to read privTags from source->extMetadata[0].privTags instead of data[0].properties, so the JSON path keeps emitting ext.metadata.privTags and both paths now source from the same place.
There was a problem hiding this comment.
Update for the final approach: I removed the continue, so EventInfo.PrivTags is now ADDITIONALLY mirrored into ext.metadata.privTags but kept in Part C. The JSON path reads it from Part C unchanged, so nothing is dropped.
PrivTags union read (EventPropertiesDecorator.hpp): only route EventInfo.PrivTags into ext.metadata when v.type == EventProperty::TYPE_INT64; non-int64 values now fall through to normal Part C handling instead of reading the inactive union member. Enum verified at EventProperty.hpp:288 and EventProperties.cpp:318. JSON path (JsonFormatter.cpp:151): the decorator now moves privTags out of Part C into record.extMetadata, so JsonFormatter reads privTags from record.extMetadata rather than data[0].properties; otherwise the JSON path would drop ext.metadata.privTags. Added EventPropertiesDecoratorTests.Decorate_PrivTags_NonInt64_FallsThroughToPartC. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| // privTags is carried in record.extMetadata (populated by EventPropertiesDecorator); | ||
| // read it from there so the JSON path matches the bond serialization path. | ||
| if (!source->extMetadata.empty() && source->extMetadata[0].privTags != 0) { | ||
| ans["ext"]["metadata"]["privTags"] = source->extMetadata[0].privTags; | ||
| } |
There was a problem hiding this comment.
Good catch. Took the simpler route: the decorator no longer moves privTags out of Part C (additive), and I reverted JsonFormatter to its original Part C read - so it is now unchanged from main and there is no JSON-path loss for the non-int64 fallback or privTags == 0.
…t JsonFormatter Round-1 moved privTags out of Part C (continue) and changed JsonFormatter to read from extMetadata, which created JSON-path edge cases (privTags lost for the non-int64 fallback and for privTags==0). Simplify: mirror EventInfo.PrivTags into ext.metadata.privTags (still guarded on TYPE_INT64) WITHOUT removing it from Part C, and revert JsonFormatter to its original Part C read. The bond path now additionally carries ext.metadata.privTags; the JSON path is unchanged from main. Updated the unit test to reflect that PrivTags is retained in Part C. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| if (record.extMetadata.empty()) | ||
| { | ||
| record.extMetadata.push_back(::CsProtocol::MetaData()); | ||
| } | ||
| record.extMetadata[0].privTags = static_cast<uint64_t>(v.as_int64); |
There was a problem hiding this comment.
Fixed - extMetadata is now the single source of truth. The decorator moves a well-typed int64 EventInfo.PrivTags into ext.metadata.privTags and no longer leaves it in Part C (the continue is back), so the Bond payload no longer carries it twice.
| // EventInfo.PrivTags is mirrored into ext.metadata.privTags ... | ||
| ASSERT_THAT(record.extMetadata, SizeIs(1)); | ||
| EXPECT_THAT(record.extMetadata[0].privTags, Eq(static_cast<uint64_t>(privTags))); | ||
| // ... and is also retained as a Part C property (the JSON path reads it from there). | ||
| EXPECT_THAT(record.data[0].properties.find(COMMONFIELDS_EVENT_PRIVTAGS), | ||
| Ne(record.data[0].properties.end())); |
There was a problem hiding this comment.
Updated - the test now asserts EventInfo.PrivTags is routed into ext.metadata.privTags and is NOT present in Part C, matching the single-source-of-truth behavior.
| 36: optional vector<Cs> extCs; | ||
| 37: optional vector<M365a> extM365a; | ||
| // TODO(privacy-parity): 38 is a best-effort ordinal for ext.metadata; confirm against canonical CS. | ||
| 38: optional vector<MetaData> extMetadata; |
There was a problem hiding this comment.
Acknowledged - the ordinal (38) and layout are intentionally best-effort and clearly marked (TODO(privacy-parity) + the PR's reviewer caveat). This is a deliberate choice for this PR: it is not meant to merge until the canonical ext.metadata wire contract is confirmed. Gating behind a feature flag is a reasonable alternative if preferred - happy to do that instead of relying on the caveat.
…tadata) Make record.extMetadata the single representation of privTags: the decorator moves a well-typed int64 EventInfo.PrivTags into ext.metadata.privTags (no longer leaving it in Part C), and JsonFormatter now emits ext.metadata.privTags from record.extMetadata whenever present. This removes the Bond-side duplication and keeps the Bond and JSON outputs aligned on one source. Non-int64 values still fall through to ordinary Part C handling. Updated the unit test accordingly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| // privTags is the single source of truth in record.extMetadata (populated by | ||
| // EventPropertiesDecorator for well-typed int64 values); emit it whenever present. | ||
| if (!source->extMetadata.empty()) { | ||
| ans["ext"]["metadata"]["privTags"] = source->extMetadata[0].privTags; | ||
| } |
There was a problem hiding this comment.
Fixed - removed the now-redundant erase of EventInfo.PrivTags from Part C in JsonFormatter. Well-typed int64 PrivTags is emitted from ext.metadata; non-int64 values are retained as an ordinary Part C property instead of being dropped on the JSON path.
…Formatter With privTags sourced from record.extMetadata, the leftover unconditional erase of COMMONFIELDS_EVENT_PRIVTAGS from Part C would drop PrivTags on the JSON path for the non-int64 fallback case (extMetadata empty, value retained as an ordinary Part C property). Remove that erase so non-int64 PrivTags is still serialized as a Part C property, while well-typed int64 values are emitted from ext.metadata. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| // privTags is the single source of truth in record.extMetadata (populated by | ||
| // EventPropertiesDecorator for well-typed int64 values); emit it whenever present. | ||
| if (!source->extMetadata.empty()) { | ||
| ans["ext"]["metadata"]["privTags"] = source->extMetadata[0].privTags; | ||
| } |
There was a problem hiding this comment.
Clarified the comment - privTags is carried in extMetadata for well-typed int64 values, while non-int64 values remain ordinary Part C properties (so it isn't strictly a single representation, as you note).
| // 37: optional vector<M365a> extM365a | ||
| std::vector< ::CsProtocol::M365a> extM365a; | ||
| // 38: optional vector<MetaData> extMetadata | ||
| std::vector< ::CsProtocol::MetaData> extMetadata; | ||
| // 41: optional vector<Data> ext | ||
| std::vector< ::CsProtocol::Data> ext; |
There was a problem hiding this comment.
Acknowledged - adding extMetadata to the public CsProtocol::Record is an intended, necessary part of carrying privTags in ext.metadata on the bond path. It is a new optional trailing field (existing members are not reordered), but it is part of the same wire/schema change this PR explicitly marks as best-effort and not-for-merge until the canonical ext.metadata contract - and its ABI/compat implications - are confirmed. Flagging it alongside the ordinal caveat rather than resolving it here.
Soften the "single source of truth" wording: privTags is carried in extMetadata for well-typed int64 values, while non-int64 values remain ordinary Part C properties. Comment-only change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Forward
EventInfo.PrivTagsintoext.metadataon the cross-platform bond (CsProtocol) serialization path, so it is carried the same way the UTC and JSON paths already carry it.Background
EventInfo.PrivTagsintoext.metadata(lib/modules/utc/utctelemetrysystem.cpp).JsonFormattermaps it toext.metadata.privTags(lib/system/JsonFormatter.cpp).TelemetrySystemserializes withBondSerializer, but the bundledCsProtocolschema has noext.metadataextension — so on that pathEventInfo.PrivTagswas only emitted as a Part C property.This change adds the missing extension and populates it on the direct path for parity.
Changes
CsProtocol.bond/CsProtocol_types.hpp: add aMetaData { privTags }extension andRecord.extMetadata(field 38), defined unconditionally (next toM365a) so it is present in the default build, not only underHAVE_CS4_FULL.CsProtocol_readers.hpp/CsProtocol_writers.hpp: (de)serialize the new extension.EventPropertiesDecorator: routeEventInfo.PrivTagsintoext.metadata.privTagsinstead of emitting it as a Part C property (mirrorsJsonFormatter).EventPropertiesDecoratorTests.Decorate_PrivTags_RoutedToExtMetadata.The
ext.metadataextension ordinal (38) and field layout are best-effort and are not yet confirmed against the canonical Common Schema. Because the bond payload is decoded by field ordinal, these must be verified against the canonical CSext.metadatadefinition (and the collector's handling of it) before being relied upon. See theTODO(privacy-parity)markers. Happy to update the ordinals/types once confirmed.Validation
CsProtocol_types/readers/writers) compiles cleanly under the default config (HAVE_CS4_FULLoff); the new extension is defined unconditionally andRecordbuilds/compares with it.EventInfo.PrivTagsis routed intorecord.extMetadata[0].privTagsand not duplicated as a Part C property.