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: App token refreshing #289

Merged
merged 4 commits into from
Aug 15, 2024

Conversation

TrishGillett
Copy link
Contributor

@TrishGillett TrishGillett commented Aug 9, 2024

This PR implements app token refreshing! Thank you for your patience with the previous refactoring PRs that prepared for this one.

  • As part of has_calls_remaining, the amount of time left until estimated token expiry is checked, and tokens are refreshed if expiry is close.
  • A new config option expiry_time_buffer is added, controlling how many minutes before expiry the app token will be refreshed. This parallels how rate_limit_buffer is used to ensure we don't push tokens all the way to their limits.

@edgarrmondragon edgarrmondragon self-requested a review August 9, 2024 20:35
@TrishGillett
Copy link
Contributor Author

👋 I ran some large extractions over the weekend using this PR's SHA and a single app key.

The good news is that it ran smoothly for several hours at a time without any authenticator related failure. The longest job I ran took about 8.5 hours, and I can see the GitHub app token refresh succeeded. logging at regular intervals.

One slightly confusing find is that whenever the expiry time approached a refresh would happen, and then another refresh would happen say 3-6 minutes later. Log excerpt demonstrating it:

2024-08-10 03:58:53,256 | INFO     | tap-github           | Tap will run with 1 auth tokens
2024-08-10 04:48:55,891 | INFO     | tap-github           | GitHub app token refresh succeeded.
2024-08-10 04:53:55,361 | INFO     | tap-github           | GitHub app token refresh succeeded.
2024-08-10 05:38:55,944 | INFO     | tap-github           | GitHub app token refresh succeeded.
2024-08-10 05:44:31,833 | INFO     | tap-github           | GitHub app token refresh succeeded.
2024-08-10 06:28:56,078 | INFO     | tap-github           | GitHub app token refresh succeeded.
2024-08-10 06:34:59,825 | INFO     | tap-github           | GitHub app token refresh succeeded.

I was extracting the issues stream, and when I look at where GitHubTokenAuthenticator is initialized it looks like maybe the parent stream (repos) and child stream (issues) are each constructing and maintaining a GitHubTokenAuthenticator object, meaning there would be two installation access tokens at any given time and each would need to be updated periodically. Does that match your mental model @edgarrmondragon? As long as we understand why it's refreshing twice and believe it isn't due to a bug, I don't think this is a problem - multiple installation access tokens can exist for the same app and will just cumulatively contribute to the app's rate limit.

@TrishGillett TrishGillett changed the title feat: App token refreshing [WIP] feat: App token refreshing Aug 12, 2024
@sicarul
Copy link
Contributor

sicarul commented Aug 14, 2024

If it helps in anyway, we've been using app tokens with this method without issues since last year, but i haven't refactored the code to address Meltano's team feedback: #201

@edgarrmondragon
Copy link
Member

Does that match your mental model @edgarrmondragon?

That makes sense to me. We could always make it singleton in the future, but I don't think it causes issues at the moment?

@TrishGillett
Copy link
Contributor Author

We could always make it singleton in the future, but I don't think it causes issues at the moment?

Agreed. 👍

Copy link

sonarcloud bot commented Aug 15, 2024

@sicarul sicarul mentioned this pull request Aug 15, 2024
@edgarrmondragon
Copy link
Member

Thanks @TrishGillett, this is a monumental improvement!

@edgarrmondragon edgarrmondragon merged commit 643b1c4 into MeltanoLabs:main Aug 15, 2024
8 checks passed
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.

3 participants