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

v1: get node_id automatically #740

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

shivdhar
Copy link

v1: Create node_id automatically from the MAC address of the machine, with fallback, according to the rules laid out by RFC 4122.

Advantage:

The user can now call now_v1_auto() and get a V1 UUID without needing to figure out how to manually retrieve/create the node_id.

Performance: Comparable

=> cargo +nightly bench --features=v1_auto -- bench_v1
running 2 tests
test bench_v1      ... bench:          65 ns/iter (+/- 15)
test bench_v1_auto ... bench:          66 ns/iter (+/- 6)

@shivdhar
Copy link
Author

Some prettification/tests/docs is remaining, but the bulk of the work is done. Feedback from maintainers would be appreciated, thanks!

@shivdhar shivdhar marked this pull request as ready for review February 25, 2024 18:45
@KodrAus
Copy link
Member

KodrAus commented Mar 3, 2024

Hi @shivdhar! Thanks for the PR. I'm not sure this is something we should add to uuid itself, but if you'd like to convert this into an example in the examples repository for producing v1 UUIDs from MAC addresses. I think that would be a great way to help users discover how to do it.

@shivdhar
Copy link
Author

shivdhar commented Mar 5, 2024

Hi @KodrAus ! I actually was surprised when I tried to use the crate and, instead of just calling now_v1, had to figure out how it worked, read the RFC, make sure the MAC address was big-endian, etc. etc. As a user, I'd just like to get the v1 UUID without worrying about how it works, wouldn't you agree?

Also I took a look at the history of the feature in this repo, and the only reason it seems it wasn't included since the beginning is this issue:

Also "Node ID" is a vague notion, usually the mac address, but I assume we'd just want to let someone pass in 16 bytes for that.

However, node ID is quite well defined in the RFC (along with fallback) so I don't see any reason why we shouldn't have a conformant implementation.

Another blocker -- way back in 2016 -- seems to have been this:

I don't think we're really poised to fully provide V1 UUID generation because we don't have a robust method of figuring out what the local MAC address is?
#59 (comment)

This seems to have been solved by the mac_address crate though fairly well; it supports most common platforms and seems to be fairly popular and good at its job (over 2.4M downloads), so we're probably in a better place to provide this in 2024.

Most other languages let us get a v1 UUID automatically without a mandatory node_id argument too, it was very surprising as a user when I couldn't do so with this, the "official" crate for Rust.

Let me know what you think!

@KodrAus
Copy link
Member

KodrAus commented Mar 18, 2024

I think discoverability is definitely an issue, but I also think that's solved by adding an example. My specific concerns with direct support are:

  1. Adding another dependency to the library that's called under-the-hood. It hides from the user how we get that address, which currently is the first non-loopback adapter, but that isn't guaranteed to be what someone wants. It also requires background initialization using OnceLocks, which we'd need to bump our MSRV or pull in another dependency for. We'd need to communicate what the lifetime of that address is, and what platforms it does and doesn't support.
  2. UUID V1 with actual MAC addresses is more of a niche nowadays (in my opinion), so I'm reluctant to add more infrastructure in the form of APIs and crate features to support them that need to be maintained.

At least I'd like to start with an example, and consider revisiting this in the future if it becomes widely desirable.

@shivdhar
Copy link
Author

shivdhar commented Jun 8, 2024

I suppose I fail to understand why a well-understood and well-supported part of the RFC spec shouldn't be a part of its implementation, and indeed hasn't been in all these years. UUIDv1 generation should be automatic and a no-brainer; MAC address integration is sufficiently complex that users shouldn't have to deal with it unless they specifically wish to tweak it. (An example in the docs adds cognitive overhead and another thing to learn about when it should be a library implementation.)

  1. mac_address is already a mature and popular library, and unlikely to keel over randomly. As for the OnceLocks, good point! Maybe encapsulate the node_id in a V1Auto struct, which gets a now() method? Would eliminate the static lifetime allocation entirely, which i quite agree should be avoided; and also obviates the need to increase MSRV.

  2. This is subjective; for what it's worth, I needed it and was surprised I couldn't get a v1 UUID out of the box, hence this PR.

I think I've already noted that the absence of this clearly defined part of the UUID v1 spec was accidental due to ecosystem immaturity back in 2017; the adding another library part is a pain I agree, but should it stop us from implementing missing functionality in this important library? I think not.

End-user impact is entirely optional and opt-in with the feature flag too. Let me know if 1. is a good idea to avoid increasing MSRV and eliminating lifetime allocation of the mac address; then I'll work on replacing it.

Thanks!

@KodrAus
Copy link
Member

KodrAus commented Jun 17, 2024

@shivdhar The method you've proposed is perfectly convenient and I appreciate you taking the time to put a PR together for it, but it simply replaces this:

let mac = mac_address::get_mac_address().unwrap().unwrap();

let uuid = uuid::Uuid::now_v1(&mac.bytes());

while also picking semantics of what happens when the address is unavailable on their behalf, without giving you an opportunity to decide what address to use, and by hiding synchronization around it. If another user wants to do any of these things in a different way then we'll either need to add more functionality to support them, or the API will simply not work for them. All together, I don't believe this is something we should support directly in uuid, but is exactly the sort of thing we have an examples directory for.

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.

2 participants