Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require that instance_uid is ULID instead of only recommending it #116

Conversation

tigrannajaryan
Copy link
Member

Resolves #113

This PR changes ULID to be a MUST in order to:

  • Avoid mistakes when the Agent chooses too short instance id and causes collisions.
  • Make the Server's life a bit easier since will know it is always a 128 bits of data and can store in places where UUIDs can be stored (e.g. Postgres has a data type UUID).

@tigrannajaryan tigrannajaryan requested a review from a team July 28, 2022 18:11
Resolves open-telemetry#113

This PR changes ULID to be a `MUST` in order to:
- Avoid mistakes when the Agent chooses too short instance id and causes collisions.
- Make the Server's life a bit easier since will know it is always a 128 bits of data and can store in places where UUIDs can be stored (e.g. Postgres has a data type UUID).
@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/make-ulid-required branch from 6746733 to 218536c Compare July 28, 2022 18:17
Copy link
Contributor

@andykellr andykellr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be a breaking change for bindplane-op agents, but I think it is a good change.

@tigrannajaryan
Copy link
Member Author

This will be a breaking change for bindplane-op agents, but I think it is a good change.

Yes. Good news is that this seems to be the last remaining known issue that will be a breaking change. We should probably wait for a bit after merging this one to see if any other issues are discovered that will be also breaking so that we can release them at once (multiple breaking releases are usually more painful - that's why we promise no more frequently than once every 3 months when in Beta).

@tigrannajaryan tigrannajaryan merged commit bbabcbe into open-telemetry:main Jul 28, 2022
@tigrannajaryan tigrannajaryan deleted the feature/tigran/make-ulid-required branch July 28, 2022 21:05
tigrannajaryan added a commit to tigrannajaryan/opamp-go that referenced this pull request Jul 29, 2022
tigrannajaryan added a commit to open-telemetry/opamp-go that referenced this pull request Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Require that instance_uid is ULID instead of only recommending it
2 participants