feat: tf api metrics#4880
Conversation
pkg/controller/tf/controller.go
Outdated
| metaC := baseConfig.Clone() | ||
| metaC.Context = metricstransport.WithControllerName(ctx, r.controllerName) // for metrics transport | ||
| meta = metaC |
There was a problem hiding this comment.
in my experience, we needed to clone this Config as the Context that it uses for HTTP requests is the one that we first configure the provider with AND NOT the ctx that we have in hand here.
0ec2d47 to
ae01764
Compare
|
/lgtm |
| meta = metaC | ||
| } | ||
|
|
||
| requeue, err := r.sync(ctx, resource, meta) |
There was a problem hiding this comment.
Seems strange you are passing 1 context directly to sync and a different context to sync through the meta object.
There was a problem hiding this comment.
Yes that's what I wanted to highlight here:
Note "we" were always doing this before in that the TF Apply command takes in the ctx for some operations but for actual HTTP calls and transport calls it uses the Context in the meta
There was a problem hiding this comment.
Still don't like this. To quote from https://go.dev/blog/context-and-structs "Contexts should not be stored inside a struct type, but instead passed to each function that needs it."
To quote from https://pkg.go.dev/context "Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it. This is discussed further in https://go.dev/blog/context-and-structs. The Context should be the first parameter, typically named ctx:"
There was a problem hiding this comment.
I realize this is not an anti-pattern your putting in, but it is very much an anti-pattern.
|
/hold want to merge #4879 first |
ae01764 to
f1f84ed
Compare
|
/hold cancel |
4879 is merged |
| RequestBatcherIam *RequestBatcher | ||
| } | ||
|
|
||
| func (c *Config) Clone() *Config { |
There was a problem hiding this comment.
Does this really need to be in the third_party directory? Could we maybe move it into the controller, and have a function like CloneAndChangeContext, and in the comment for that we say what it is doing, how this is sort of a hack, but it's a reasonable thing to do given what we have to work with.
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
This reverts commit 1136273.
2123e27 to
c1db92b
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheftako The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
eca274e
into
GoogleCloudPlatform:master
…i-metrics-tfs feat: tf api metrics
Change description
Adds support for tf level metrics too!
Special notes for your reviewer:
Does this PR add something which needs to be 'release noted'?
Additional documentation e.g., references, usage docs, etc.:
Intended Milestone
Please indicate the intended milestone.
Tests you have done
make ready-prto ensure this PR is ready for review.