-
Notifications
You must be signed in to change notification settings - Fork 512
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
Prepare for context argument changes in Prometheus #6097
Conversation
Prepare for context argument changes (some functions gain a context argument, while others lose it) in Prometheus, by adding a context argument to functions that will need it. Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
ab87dc5
to
2a64b04
Compare
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks for the careful reviews @krajorama and @dimitarvdimitrov! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for addressing my comments
@@ -830,7 +830,7 @@ func (i *Ingester) PushWithCleanup(ctx context.Context, pushReq *push.Request) ( | |||
return &mimirpb.WriteResponse{}, nil | |||
} | |||
|
|||
db, err := i.getOrCreateTSDB(userID, false) | |||
db, err := i.getOrCreateTSDB(ctx, userID, false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're passing the request context to getOrCreateTSDB()
. I don't know what it will be used for, but if the context will be retained by TSDB then I think this is a mistake. We shouldn't pass any request-specific context to any place where the context will be retained.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback @pracucci. The context shouldn't be retained, but I'm looking into whether it's a bad idea to pass it down into TSDB.
What this PR does
Prepare for context argument changes (some functions gain a context argument, while others lose it) in Prometheus, by adding a context argument to functions that will need it.
This PR should cause no behavioral changes, it is only supposed to reduce the number of changes required for updating our Prometheus dependency.
Which issue(s) this PR fixes or relates to
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]