Replace ULIDs by 16 byte ids and recommend UUID v7 #186
Conversation
Implements spec change open-telemetry/opamp-spec#186 The example server still accepts old-style ULID agent instances to demonstrate how this change can be handled in a backward compatible way.
Implements spec change open-telemetry/opamp-spec#186 The example server still accepts old-style ULID agent instances to demonstrate how this change can be handled in a backward compatible way.
|
Corresponding implementation in opamp-go: open-telemetry/opamp-go#272 |
|
@andykellr I am going to make a 0.9.0 release of the spec first, so that this change if merged goes into the next 0.10.0 release. |
|
@andykellr I want to keep opamp-go examples interoperable with Bindplane OP. If we go ahead with this and then also merge open-telemetry/opamp-go#272 do you think you will be able to quickly make Bindplane compatible with the new spec so that opamp-go client example can work with it? |
The early version of the spec was written before UUID v7 existed and ULID was chosen as a ID type that is suited to be used as a primary key in databases. Since than UUID v7 was standardized and aims to serve the exact same niche. ## Proposal Replace instance_id definition to be an arbitrary sequence of 16 bytes. Both ULID and any version of UUID may be used as the value. We also recommend to use UUID v7 for the value. This is a breaking change in the spec. We recommend opamp-go implementation to implement the change in backward compatible way, supporting both old and new instance_uid formats for a period of time. Since `bytes` and `string` are compatible in the Protobuf wire encoding it is possible to use the new Protobuf declarations to receive messages encoded using the old Protobuf declarations - string's UTF8 bytes will simply populate the bytes of the new field. Receivers can simply probe for the length of the received bytes field and if it is 26 treat it as ULID in canonical format, if it is 16 bytes treat it as opaque id in new format, otherwise consider it invalid. I will also post a PR in opamp-go that implements the above behavior.
tigrannajaryan
force-pushed
the
feature/tigran/ulid-uuid
branch
from
7c4c461
to
58efebb
Compare
Bindplane only requires a string. |
@andykellr Will it work correctly if the string contains arbitrary bytes? Protobuf spec says strings must be UTF8 compliant although not all receiver implementations validate it. |
Interesting. It sounds like this will present a problem until we expect the id to be an arbitrary array of 16 bytes. |
@andykellr I am not sure Go protobuf deserializer validates this. It may be worth checking. If there is no validation it will be seen as just another string (possible with invalid UTF8 sequences). |
I did check this against BindPlane, looks like the deserializer does indeed validate that the string is UTF-8. It'll definitely be a priority for us to update after this and open-telemetry/opamp-go#272 is merged; I think our goal would be to have BindPlane updated to support a week or 2 after it's released. |
@BinaryFissionGames @andykellr OK, I think we are ready here in the spec and in opamp-go. If the timing works for you we can go ahead and merge this PR. |
|
@tigrannajaryan Yep, we're good with merging this 👍 |
Updates OpAMP spec to 0.9.0 and implements spec change open-telemetry/opamp-spec#186 The example server still accepts old-style ULID agent instances to demonstrate how this change can be handled in a backward compatible way. Replace ULIDs by 16 byte ids and recommend UUID v7 (open-telemetry#186)
Updates OpAMP spec to 0.9.0 and implements spec change open-telemetry/opamp-spec#186 The example server still accepts old-style ULID agent instances to demonstrate how this change can be handled in a backward compatible way. Replace ULIDs by 16 byte ids and recommend UUID v7 (#186)
The early version of the spec was written before UUID v7 existed and ULID was chosen as a ID type that is suited to be used as a primary key in databases. Since than UUID v7 was standardized and aims to serve the exact same niche.
Proposal
Replace instance_id definition to be an arbitrary sequence of 16 bytes. Both ULID and any version of UUID may be used as the value. We also recommend to use UUID v7 for the value.
This is a breaking change in the spec. We recommend opamp-go implementation to implement the change in backward compatible way, supporting both old and new instance_uid formats for a period of time.
Since
bytesandstringare compatible in the Protobuf wire encoding it is possible to use the new Protobuf declarations to receive messages encoded using the old Protobuf declarations - string's UTF8 bytes will simply populate the bytes of the new field. Receivers can simply probe for the length of the received bytes field and if it is 26 treat it as ULID in canonical format, if it is 16 bytes treat it as opaque id in new format, otherwise consider it invalid.I will also post a PR in opamp-go that implements the above behavior.