opentelemetry-collector-contrib icon indicating copy to clipboard operation
opentelemetry-collector-contrib copied to clipboard

[cmd/opampsupervisor]: Persist the instance ID between restarts

Open BinaryFissionGames opened this issue 10 months ago • 3 comments

Description: <Describe what has changed.>

  • Adds the ability to persist the instance ID on disk and load it on startup.

Link to tracking Issue: Closes #21073

Testing:

  • Add unit tests for persistence functionality
  • Add e2e tests for persistence (testing that initial id is persisted, and that server specified ID is persisted)

Documentation: N/A

BinaryFissionGames avatar Apr 23 '24 06:04 BinaryFissionGames

Thanks for opening this and fixing a few race conditions. Do you want a review while this is still a draft, or would you like to wait until it's marked "ready for review"?

Do you think there's any overlap with https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/30807?

evan-bradley avatar Apr 23 '24 12:04 evan-bradley

@evan-bradley It's ready for review now, I tend to open things in draft and come back to them with a fresh mind to look back over the PR, turns out it was a good idea because I caught a few weird naming things here 😆

I do think there's a small bit of overlap in #30807 - but I think it's only in the configuration. I think it makes sense to persist those statuses in different files, just because they are raw byte payloads that probably wouldn't be so useful in a structured yaml file like this.

BinaryFissionGames avatar Apr 23 '24 14:04 BinaryFissionGames

@tigrannajaryan is this ready to be merged from your perspective?

bogdandrutu avatar May 10 '24 03:05 bogdandrutu

@tigrannajaryan is this ready to be merged from your perspective?

User-visible functionality LGTM. I didn't review the code, but @evan-bradley did so I believe it should be good to merge.

tigrannajaryan avatar May 10 '24 18:05 tigrannajaryan