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-flavored ingest keys #237

Merged
merged 5 commits into from
Feb 27, 2024
Merged

Conversation

jharley
Copy link
Contributor

@jharley jharley commented Feb 27, 2024

Which problem is this PR solving?

We've now released Ingest Keys, but in order for them to work with Classic environments properly we need to update the key detection logic.

Short description of the changes

  • updates the Classic API Key detection logic to understand the shape of a Classic Ingest Key
  • makes the previously private Config.isClassic a public method (Config.IsClassic) to allow other projects to use this method and reduce the duplication of logic (i.e. beeline-go)

@jharley jharley added type: enhancement version: bump minor A PR that adds behavior, but is backwards-compatible. labels Feb 27, 2024
@jharley jharley requested a review from a team February 27, 2024 13:32
@jharley jharley requested a review from a team as a code owner February 27, 2024 13:32
Copy link
Contributor

@kentquirk kentquirk left a comment

Choose a reason for hiding this comment

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

I was slightly worried about performance, and took a minute to run a couple of benchmarks with variations on the regexp.

First, classic keys are hex only, so you can change the classic key regex to a-f instead of a-z. This makes that regex faster.

Second, if you check length manually and then remove the counts from the regexp, it runs literally 1000x faster (7 nS instead of 7 uS!)

	var classicKeyRegex = regexp.MustCompile(`^[a-f0-9]*$`)
	var classicIngestKeyRegex = regexp.MustCompile(`^hc[a-z]ic_[a-z0-9]*$`)

		if len(key) == 0 {
                       return true
		} else if len(key) == 32 {
			return classicKeyRegex.MatchString(key)
		} else if len(key) == 64 {
			return classicIngestKeyRegex.MatchString(key)
		} 
                 return false

@jharley
Copy link
Contributor Author

jharley commented Feb 27, 2024

Second, if you check length manually and then remove the counts from the regexp, it runs literally 1000x faster (7 nS instead of 7 uS!)

Whoa! Thank you for this: that's huge. Will refactor the shape of the function

@jharley jharley merged commit 60fd25a into main Feb 27, 2024
3 checks passed
@jharley jharley deleted the jharley.classic-ik-support branch February 27, 2024 17:19
jharley added a commit that referenced this pull request Mar 4, 2024
## Which problem is this PR solving?

As a follow on to #237, we now realize it's more helpful to be able to
determine if a key is a classic key just by the string and not just in
the context of a `Config`.

## Short description of the changes

- Adds a new public function `IsClassicKey()` and updates
`Config.IsClassic()` to use this without making a breaking change.
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.

4 participants