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

feat: support classic ingest keys #406

Merged
merged 5 commits into from
Feb 28, 2024

Conversation

cewkrupa
Copy link
Contributor

@cewkrupa cewkrupa commented Feb 26, 2024

Which problem is this PR solving?

We've released Ingest Keys, but need to update the key detection logic to allow Ingest Keys to be used to send data to Classic environments.

Short description of the changes

  • updates the Classic API Key detection logic to understand the shape of a Classic Ingest Key
  • Changes isClassic from a "private" class method of Libhoney to a static class method, so we can reuse it in dependent libraries without repeating logic everywhere

@MikeGoldsmith MikeGoldsmith added type: enhancement version: bump minor A PR that adds behavior, but is backwards-compatible. labels Feb 27, 2024
@MikeGoldsmith
Copy link
Contributor

Looks good and matches the other implementations. Just need to update the PR body 👍🏻

isClassic(key) {
return key.length === 32;
static isClassic(key) {
if (key === null || key === undefined || key.length === 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

other libhoneys, such as libhoney-java or libhoney-go, have a check that says if there is no key, it counts as classic. I've updated here to match, but given that this check wasn't here before I'm not sure if we want to keep it.

Comment on lines +287 to +291
else if(key.length === 32) {
return classicKeyRegex.test(key);
} else if(key.length === 64) {
return ingestClassicKeyRegex.test(key);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I setup + ran some benchmarks locally using jest-bench. I didn't include the setup in this PR (but would be happy to). The results:

Base (what was here before, just the length check)

  isClassic check
    full ingest key string, non classic    0.003 μs ±  0.80 %  (94 runs sampled)
    ingest key id, non classic             0.003 μs ±  1.02 %  (93 runs sampled)
    full ingest key string, classic        0.003 μs ±  0.90 %  (94 runs sampled)
    ingest key id, classic                 0.003 μs ±  1.09 %  (91 runs sampled)
    v2 configuration key                   0.003 μs ±  1.08 %  (94 runs sampled)
    classic key                            0.003 μs ±  0.91 %  (94 runs sampled)

With this change:

  isClassic check
    full ingest key string, non classic    0.011 μs ±  0.47 %   (98 runs sampled)
    ingest key id, non classic             0.013 μs ±  0.24 %   (99 runs sampled)
    full ingest key string, classic        0.046 μs ±  0.33 %   (95 runs sampled)
    ingest key id, classic                 0.013 μs ±  0.50 %   (99 runs sampled)
    v2 configuration key                   0.004 μs ±  0.24 %   (92 runs sampled)
    classic key                            0.032 μs ±  0.37 %  (100 runs sampled)

Copy link
Contributor

Choose a reason for hiding this comment

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

I want to make sure there's no confusion -- what I benchmarked was one implementation of a regular expression that used counting ({58}) vs one that didn't depend on counting. Pulling out the counting is what made it fast.

It looks like here you're comparing counting only with counting + regexp, which is going to be slower. That said, sub-microsecond times seem fine to me. It would be interesting to see the same benchmark data for asking the regexp to do the counting.

In any case, for Go the regexp is precompiled, and the Go regexp state machine is specifically designed to be different from those in many other languages. The same optimizations might not apply - but maybe they do. It's worth checking.

@cewkrupa cewkrupa marked this pull request as ready for review February 27, 2024 17:46
@cewkrupa cewkrupa requested a review from a team as a code owner February 27, 2024 17:46
@cewkrupa cewkrupa requested a review from a team February 27, 2024 17:47
@cewkrupa cewkrupa merged commit 8590867 into main Feb 28, 2024
4 checks passed
@cewkrupa cewkrupa deleted the cewkrupa/support-classic-ingest-keys branch February 28, 2024 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement version: bump minor A PR that adds behavior, but is backwards-compatible.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants