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

Allow an isolate to have a simple random uuid #280

Merged
merged 1 commit into from
Jan 19, 2023
Merged

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jan 17, 2023

The jsg::Lock::getUuid() will lazily create a random uuid for the
isolate instance. It is expected that this will primarily be used for
logging/diagnostics in DO cases. The uuid is generated lazily so there
is zero cost if the uuid is never used.

( Prompted by a chat with @bcaimano )

Another option would be to just log the memory address of the isolate itself but the requirement here is that the uniqueness be relatively guaranteed for the lifetime of the process, which we can't guarantee with the pointer address. We could also use a simple counter but that would likely require a lock. lazily creating the uuid here seems Good Enough (tm).

@jasnell jasnell requested a review from bcaimano January 17, 2023 23:04
@jasnell jasnell changed the title Validate addresses passed to connect. Allow an isolate to have a simple random uuid Jan 17, 2023
@jasnell jasnell force-pushed the jsnell/isolate-uuid branch from 6aea48c to 215266e Compare January 17, 2023 23:05
src/workerd/jsg/setup.c++ Outdated Show resolved Hide resolved
@jasnell jasnell force-pushed the jsnell/isolate-uuid branch from 215266e to 104bee7 Compare January 18, 2023 15:48
Copy link
Contributor

@bcaimano bcaimano left a comment

Choose a reason for hiding this comment

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

Thank you for getting to this so quickly!

So I'm trying to conceive of how I'd get this uuid to the metrics/observer system since they usually try to avoid taking the isolate lock. It feels like the IsolateObserver would be the most natural place, but that is constructed before the isolate. Perhaps we want to optionally take a uuid in isolate construction so that we could give it to both observer and isolate? We do naturally take the isolate lock immediately after construction, so I could see a world where we just stash it in other metrics types with a setter. I could also see something like tryGetUuid() that doesn't require the lock but returns a maybe. What do you think?

@jasnell
Copy link
Member Author

jasnell commented Jan 18, 2023

Exposing on js::Lock is just a convenience for some cases. You can get the uuid without acquiring the lock by going directly to IsolateBase... e.g. IsolateBase::from(isolate).getUuid()

@jasnell jasnell force-pushed the jsnell/isolate-uuid branch from 104bee7 to 36e70e7 Compare January 19, 2023 00:44
Copy link
Contributor

@bcaimano bcaimano left a comment

Choose a reason for hiding this comment

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

Hmmmm, looks like there is a compile failure for the test? Seems like a silly thing with type naming though.

I think this will serve well enough for me to start trying to use it for metrics, thank you!

@jasnell
Copy link
Member Author

jasnell commented Jan 19, 2023

Yeah I'll fix that test up in a bit. Can you look at the internal PR also so I can get this landed once CI comes up green?

@jasnell jasnell force-pushed the jsnell/isolate-uuid branch from 36e70e7 to 423092c Compare January 19, 2023 20:52
The `jsg::Lock::getUuid()` will lazily create a random uuid for the
isolate instance. It is expected that this will primarily be used for
logging/diagnostics in DO cases. The uuid is generated lazily so there
is zero cost if the uuid is never used.
@jasnell jasnell force-pushed the jsnell/isolate-uuid branch from 423092c to f24aad8 Compare January 19, 2023 21:17
@jasnell jasnell merged commit fe84260 into main Jan 19, 2023
@jasnell jasnell deleted the jsnell/isolate-uuid branch January 19, 2023 21:45
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