search

open-telemetry / opamp-spec

OpAMP Specification
Apache License 2.0
102 stars 33 forks source link

Replace ULIDs by 16 byte ids and recommend UUID v7 #186

Closed tigrannajaryan closed 4 months ago

tigrannajaryan commented 4 months ago

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 commented 4 months ago

Corresponding implementation in opamp-go: https://github.com/open-telemetry/opamp-go/pull/272

tigrannajaryan commented 4 months ago

@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.

tigrannajaryan commented 4 months ago

@andykellr I want to keep opamp-go examples interoperable with Bindplane OP. If we go ahead with this and then also merge https://github.com/open-telemetry/opamp-go/pull/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?

andykellr commented 4 months ago

@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?

Bindplane only requires a string.

tigrannajaryan commented 4 months ago

@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?

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.

andykellr commented 4 months ago

@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?

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.

tigrannajaryan commented 4 months ago

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).

BinaryFissionGames commented 4 months ago

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 https://github.com/open-telemetry/opamp-go/pull/272 is merged; I think our goal would be to have BindPlane updated to support a week or 2 after it's released.

tigrannajaryan commented 4 months ago

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.

BinaryFissionGames commented 4 months ago

@tigrannajaryan Yep, we're good with merging this 👍