Skip to content

Conversation

@alexgallotta
Copy link
Contributor

@alexgallotta alexgallotta commented Sep 16, 2024

I went though multiple implementations and eventually settled for OnceLock:

  • using RWLock there are no guarantees that the producer will be done before the consumers are called, risking deadlocks, same for broadcast channels
  • https://doc.rust-lang.org/stable/std/sync/struct.OnceLock.html can be used to initialize the runtime tag value and be passed around behind a ARC. OnceLock won't block on get and can be initialize once. If the value is not ready, the tag won't be added for that datapoint. Datadog aggregation should handle and fill that properly
  • the runtime resolution logic will contains retry and such, it's the easy part. Not yet added
  • tag provider is used as a struct in different ways, adding anything async there will exponentially complicate the logic and force huge refactors. It's probably a wrong mix of data and logic
  • dogstatsd will need changes too, so another PR in libdatadog must be added. This will be a breaking change. To avoid further issue, the OnceLock will have a struct rather than String so more async values can be added later

Copy link
Collaborator

@blt blt left a comment

Choose a reason for hiding this comment

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

I buy the use of a OnceLock. Tidy stuff. I'm missing in this PR where you are seting or try_inserting but I've been travelling and am sleepy so I might just be missing it.

I would also note that you might be able to elide the use of the Arc by having access to the runtime be mediated through a function, have that function interact with a static OnceLock. That would change your testing strategy some but it would save needing to pass an Arc around, dunno that it would have much impact on runtime behavior. It's a trick I like in my work though.

@alexgallotta alexgallotta deleted the SVLS-4904-async-runtime-resolution branch September 18, 2024 18:13
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.

3 participants