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

Generate default client IDs using host and client start time #255

Merged
merged 1 commit into from
Mar 9, 2024

Conversation

brandur
Copy link
Contributor

@brandur brandur commented Mar 8, 2024

Here, move away from ULIDs for generating client IDs, and over to a
custom ID generator that uses a combination of host and client start
time. This has two benefits:

  • Host/start time are more descriptive for humans. It should give an
    operator a rough idea as to where the name of a client originated
    from, and if it's a client that's likely long dead (if the timestamp
    is very old).

  • Lets us drop our ULID package dependency, which we don't use for
    anything else.

Generated IDs look like this for example:

miniml2_local_2024_03_07T04_39_12

I've tried to arrange them so that on most systems you'll be able to
double click on an ID to select the whole thing, which might be helpful
while copying information or debugging a production problem. Characters
like colons (:) are less friendly for double-click-to-select, so I've
avoided them. Hyphens and dots aren't universally friendly for double
click either (depends on program and configuration), so I've avoided
those as well.

@brandur brandur requested a review from bgentry March 8, 2024 02:47
@bgentry
Copy link
Contributor

bgentry commented Mar 8, 2024

Generated IDs look like this for example:

miniml2.local_2024-03-07T04-39-12

I've tried to arrange them so that on most systems you'll be able to double click on an ID to select the whole thing, which might be helpful while copying information or debugging a production problem. Characters like colons (:) are less friendly for double-click-to-select, so I've avoided them. Hyphens aren't universally friendly for double click either (depends on program and configuration), but they're more so.

Hmm, macOS terminal and Chrome both want to break on the . and - characters in the above, so unfortunately it's pretty far from double-clickable 😕

Copy link
Contributor

@bgentry bgentry left a comment

Choose a reason for hiding this comment

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

LGTM but probably warrants a changelog entry ✌️

@brandur
Copy link
Contributor Author

brandur commented Mar 8, 2024

Hmm, macOS terminal and Chrome both want to break on the . and - characters in the above, so unfortunately it's pretty far from double-clickable 😕

OOC, are you actually using macOS terminal? I would've thought most River devs on Mac would be using iTerm, which does allow double-click across hyphens.

It's definitely very system/app dependent though.

@brandur brandur force-pushed the brandur-default-client-id-using-host-and-time branch 2 times, most recently from 7c47bd0 to cc2b765 Compare March 9, 2024 16:20
Here, move away from ULIDs for generating client IDs, and over to a
custom ID generator that uses a combination of host and client start
time. This has two benefits:

* Host/start time are more descriptive for humans. It should give an
  operator a rough idea as to where the name of a client originated
  from, and if it's a client that's likely long dead (if the timestamp
  is very old).

* Lets us drop our ULID package dependency, which we don't use for
  anything else.

Generated IDs look like this for example:

    miniml2_local_2024_03_07T04_39_12

I've tried to arrange them so that on most systems you'll be able to
double click on an ID to select the whole thing, which might be helpful
while copying information or debugging a production problem. Characters
like colons (`:`) are less friendly for double-click-to-select, so I've
avoided them. Hyphens and dots aren't universally friendly for double
click either (depends on program and configuration), so I've avoided
those as well.
@brandur brandur force-pushed the brandur-default-client-id-using-host-and-time branch from cc2b765 to 9415222 Compare March 9, 2024 16:22
@brandur
Copy link
Contributor Author

brandur commented Mar 9, 2024

@bgentry Changed this to be all underscores instead like:

miniml2_local_2024_03_07T04_39_12

And added changelog.

@brandur brandur merged commit 4bfe88a into master Mar 9, 2024
8 of 10 checks passed
@brandur brandur deleted the brandur-default-client-id-using-host-and-time branch March 9, 2024 16:25
@bgentry
Copy link
Contributor

bgentry commented Mar 10, 2024

Changed this to be all underscores instead like:

miniml2_local_2024_03_07T04_39_12

And added changelog.

I think that's good at least to preserve double-click selectability in ~all platforms. People can customize this to their heart's desire so we don't need to stress too much about it IMO. They can use pod/container names, dyno names, etc.

@brandur
Copy link
Contributor Author

brandur commented Mar 10, 2024

I think that's good at least to preserve double-click selectability in ~all platforms. People can customize this to their heart's desire so we don't need to stress too much about it IMO. They can use pod/container names, dyno names, etc.

Yep, 100%. My thoughts too.

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