open-telemetry / opamp-spec

OpAMP Specification
Apache License 2.0
102 stars 33 forks source link

Status compression is complicated and confusing #101

Closed tigrannajaryan closed 2 years ago

tigrannajaryan commented 2 years ago

Status compression is quite complicated and confusing. It uses 4 different hashes and lots of code to enable compression and detection of lost messages (during server restarts).

We can simplify this significantly by doing this instead:

Here is a draft PR demonstrate how it works: https://github.com/open-telemetry/opamp-go/pull/93 It is more than 100 lines of code removed.

tigrannajaryan commented 2 years ago

@andykellr @pmm-sumo I am doing a simplification pass over the spec in order to remove unnecessary complexity.

The whole hashes and status compression is unnecessarily complicated. I made a PR with a simpler approach.

Please let me know what you think.

andykellr commented 2 years ago

I think you're right. Sadly our original server implementation ignored the hashes because it was complicated and so I spent the last 2 days respecting the hashes and adding tests to ensure proper behavior. It wasn't easy but using go generics I was able to consolidate much of the hash logic. That said, I think this is a good simplification.

Because this is a significant breaking change (protobuf ids all shift), I would love to push this through if there is agreement so that we can merge this into our agent and server implementations.