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

Improve context propagation to Scalers #2190

Closed
jerbob92 opened this issue Oct 13, 2021 · 6 comments · Fixed by #2267
Closed

Improve context propagation to Scalers #2190

jerbob92 opened this issue Oct 13, 2021 · 6 comments · Fixed by #2267
Labels
feature-request All issues for new features that have not been committed to needs-discussion

Comments

@jerbob92
Copy link
Contributor

jerbob92 commented Oct 13, 2021

Proposal

At this point, the scalers only receive a context in the scaler methods GetMetrics() and IsActive().

I think there are 2 issues that we face right now:

  1. A lot of Scalers don't actually use the available context, while a lot of them just do HTTP requests which can use context just fine (just use http.NewRequestWithContext() instead of http.NewRequest()).
  2. There is no context available in buildScaler, while some Scalers would be able to use a context there (the Redis Scaler pings the Redis server in the buildScaler logic)

The first issue is quite easy to fix, every Scaler should be checked whether it can use a context in GetMetrics() or IsActive() and then implement that. If it already uses it we should check if it uses it properly. (for example, MongoDB scaler does use a context in the CountDocuments() method, but it extends context.Background() instead of the given context in GetMetrics() and IsActive()).

The second issue is harder. If you look at the code in scale_handler.go, if you go back far enough from buildScaler, you eventually find a context in startPushScalers and startScaleLoop. This is the cancelable context from HandleScalableObject. The question would be if that is the right context to pass to the buildScaler, or whether we have to introduce a new cancelable context.

Use-Case

A context is a useful feature of Go to pass arbitrary values around, but it also provides features like canceling and timeouts. A lot of libraries allow you to pass a context to it when doing stuff, for example:

  • http.NewRequestWithContext()
  • redis-go, in all it's commands
  • PostgreSQL/MySQL/MSSQL Scaler (due to SQL's QueryRowContext)
  • MongoDB
  • Probably more that I didn't see

If we would use context's properly everywhere, Scalers have to spend less time on tasks that would otherwise timeout/cancel.

Anything else?

Anything to add @zroubalik?

@jerbob92 jerbob92 added feature-request All issues for new features that have not been committed to needs-discussion labels Oct 13, 2021
@jerbob92 jerbob92 mentioned this issue Oct 13, 2021
5 tasks
@zroubalik
Copy link
Member

@jerbob92 thanks for opening this, well written.

@zroubalik
Copy link
Member

@ahmelsayed do you think that you can prepare the code in #2187 so we can later resolve the second issue mentioned here.

@arschles
Copy link
Contributor

@jerbob92 @zroubalik I can take on the first point in the original post. Is that ok?

@zroubalik
Copy link
Member

@arschles that would be great!

@arschles
Copy link
Contributor

work started in #2202

@arschles
Copy link
Contributor

arschles commented Nov 9, 2021

Now that #2187 is merged, I can take (2). I'll submit a draft PR today to start it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request All issues for new features that have not been committed to needs-discussion
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants