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 #180

Merged
merged 13 commits into from
Mar 6, 2024

Conversation

brookesargent
Copy link
Contributor

@brookesargent brookesargent commented Mar 5, 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
  • updates tests with a more comprehensive list of cases

How to verify that this has the expected result

It should be possible to use an ingest key pointed to a classic environment after this change. Please note that the ability to create ingest keys in classic environments isn't yet publicly available (since we need to update this library and others to understand classic ingest keys).

@brookesargent brookesargent marked this pull request as ready for review March 5, 2024 19:59
@brookesargent brookesargent requested a review from a team as a code owner March 5, 2024 19:59
@brookesargent brookesargent requested a review from a team March 5, 2024 19:59
@brookesargent brookesargent self-assigned this Mar 5, 2024
@brookesargent brookesargent added merge at will Reviewer can merge the PR once reviewed. type: enhancement New feature or request version: bump minor A PR that adds behavior, but is backwards-compatible. labels Mar 5, 2024
Copy link

@jharley jharley left a comment

Choose a reason for hiding this comment

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

LGTM! looks like the function is only used by the local exporter so no real performance impact

Copy link
Contributor

@JamieDanielson JamieDanielson left a comment

Choose a reason for hiding this comment

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

Thanks! Added a suggestion to update the code comment defining is_classic to include the logic being used for ingest keys, otherwise LGTM

@@ -98,7 +99,13 @@ def is_classic(apikey: str) -> bool:
Returns:
bool: true if the api key is a classic key, false if not
"""
return apikey and len(apikey) == 32
if not apikey:
Copy link
Contributor

Choose a reason for hiding this comment

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

fancy! Can we update the comment to mention the new logic for ingest keys?

@JamieDanielson
Copy link
Contributor

looks like the function is only used by the local exporter so no real performance impact

It's also used in setting the traces headers but it's probably still fine

@brookesargent brookesargent merged commit dba8928 into main Mar 6, 2024
3 checks passed
@brookesargent brookesargent deleted the brooke.support-classic-ingest-keys branch March 6, 2024 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge at will Reviewer can merge the PR once reviewed. type: enhancement New feature or request 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.

3 participants