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.

Sign up for GitHub

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

Jump to bottom

[specific ci=Group23-Vic-Machine-Service] Use trace.NewOperation pattern in vic-machine service API handlers #6563

Merged
merged 5 commits into from
Oct 20, 2017
Merged

[specific ci=Group23-Vic-Machine-Service] Use trace.NewOperation pattern in vic-machine service API handlers #6563

merged 5 commits into from
Oct 20, 2017

Conversation

AngieCris
Copy link
Contributor

Fixes #6374

@AngieCris AngieCris self-assigned this Oct 13, 2017
@AngieCris AngieCris requested review from jzt and zjs October 13, 2017 20:14
@AngieCris AngieCris requested a review from emlin October 13, 2017 20:15
jzt
jzt reviewed Oct 18, 2017
View reviewed changes
lib/apiservers/service/restapi/handlers/vch_create.go Outdated
@@ -77,17 +78,19 @@ func (h *VCHCreate) Handle(params operations.PostTargetTargetVchParams, principa
return operations.NewPostTargetTargetVchDefault(util.StatusCode(err)).WithPayload(&models.Error{Message: err.Error()})
}

validator, err := validateTarget(params.HTTPRequest.Context(), d)
op := trace.NewOperation(params.HTTPRequest.Context(), "vch create handler")

Copy link
Contributor

Choose a reason for hiding this comment

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

Minor, but for consistency with the other files, can we remove this empty line?

jzt
jzt reviewed Oct 18, 2017
View reviewed changes
lib/apiservers/service/restapi/handlers/vch_create.go Outdated
@@ -113,17 +116,19 @@ func (h *VCHDatacenterCreate) Handle(params operations.PostTargetTargetDatacente
return operations.NewPostTargetTargetDatacenterDatacenterVchDefault(util.StatusCode(err)).WithPayload(&models.Error{Message: err.Error()})
}

validator, err := validateTarget(params.HTTPRequest.Context(), d)
op := trace.NewOperation(params.HTTPRequest.Context(), "vch create handler")

Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

jzt
jzt reviewed Oct 19, 2017
View reviewed changes
Copy link
Contributor

@jzt jzt left a comment

Choose a reason for hiding this comment

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

Sorry I didn't notice this on the first pass, but in my own related task I've noticed a couple of things:

One is that there are calls to functions that take context.Context and we are passing op.Context when just op would work.

The other is that those receiving functions are taking context.Context when they should be taking trace.Operation.

We can kill these two birds with one stone by a) passing op around as op (vs. op.Context) and b) refactoring functions in the call chain to accept trace.Operation instead of context.Context. This would allow us to use op when logging messages, which will give us more clarity when debugging in concurrent scenarios.

It's probably outside of the scope of the issue associated with this PR. If it is, just let me know and I'll create a new issue around this that we can tackle later.

@AngieCris
Copy link
Contributor Author

@jzt thanks a lot for the comment, makes perfect sense
just pushed the change, can you take a look? Thanks!

@jzt
Copy link
Contributor

jzt commented Oct 19, 2017

LGTM, thanks @AngieCris!

mdharamadas1
mdharamadas1 approved these changes Oct 20, 2017
View reviewed changes
Copy link
Contributor

@mdharamadas1 mdharamadas1 left a comment

Choose a reason for hiding this comment

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

lgtm

@AngieCris AngieCris merged commit d75e610 into vmware:feature/vic-machine-service Oct 20, 2017
@AngieCris AngieCris mentioned this pull request Oct 20, 2017
Closed
zjs pushed a commit that referenced this pull request Oct 25, 2017
@AngieCris @zjs
Use trace.NewOperation pattern in vic-machine service API handlers (#…
d1578e2 
…6563)

* trace.NewOperation pattern used for vic-machine service API

* whitespace

* whitespace

* clean up trace.op pattern in API handler functions

* bug fix: validateTarget takes in context since used by buildData
zjs pushed a commit that referenced this pull request Oct 31, 2017
@AngieCris @zjs
Use trace.NewOperation pattern in vic-machine service API handlers (#…
05aa4dd 
…6563)

* trace.NewOperation pattern used for vic-machine service API

* whitespace

* whitespace

* clean up trace.op pattern in API handler functions

* bug fix: validateTarget takes in context since used by buildData
zjs pushed a commit that referenced this pull request Nov 6, 2017
@AngieCris @zjs
Use trace.NewOperation pattern in vic-machine service API handlers (#…
0f42ef6 
…6563)

* trace.NewOperation pattern used for vic-machine service API

* whitespace

* whitespace

* clean up trace.op pattern in API handler functions

* bug fix: validateTarget takes in context since used by buildData
zjs pushed a commit that referenced this pull request Nov 6, 2017
@AngieCris @zjs
Use trace.NewOperation pattern in vic-machine service API handlers (#…
7fa52b1 
…6563)

* trace.NewOperation pattern used for vic-machine service API

* whitespace

* whitespace

* clean up trace.op pattern in API handler functions

* bug fix: validateTarget takes in context since used by buildData
zjs pushed a commit that referenced this pull request Nov 6, 2017
@AngieCris @zjs
Use trace.NewOperation pattern in vic-machine service API handlers (#…
3fd1d4b 
…6563)

* trace.NewOperation pattern used for vic-machine service API

* whitespace

* whitespace

* clean up trace.op pattern in API handler functions

* bug fix: validateTarget takes in context since used by buildData
zjs pushed a commit to zjs/vic that referenced this pull request Nov 7, 2017
@AngieCris @zjs
Use trace.NewOperation pattern in vic-machine service API handlers (v…
90b7daf 
…mware#6563)

* trace.NewOperation pattern used for vic-machine service API

* whitespace

* whitespace

* clean up trace.op pattern in API handler functions

* bug fix: validateTarget takes in context since used by buildData
zjs pushed a commit to zjs/vic that referenced this pull request Nov 7, 2017
@AngieCris @zjs
Use trace.NewOperation pattern in vic-machine service API handlers (v…
9a45fd3 
…mware#6563)

* trace.NewOperation pattern used for vic-machine service API

* whitespace

* whitespace

* clean up trace.op pattern in API handler functions

* bug fix: validateTarget takes in context since used by buildData
zjs pushed a commit that referenced this pull request Nov 15, 2017
@AngieCris @zjs
Use trace.NewOperation pattern in vic-machine service API handlers (#…
e8224ba 
…6563)

* trace.NewOperation pattern used for vic-machine service API

* whitespace

* whitespace

* clean up trace.op pattern in API handler functions

* bug fix: validateTarget takes in context since used by buildData
zjs pushed a commit that referenced this pull request Nov 16, 2017
@AngieCris @zjs
Use trace.NewOperation pattern in vic-machine service API handlers (#…
be4902d 
…6563)

* trace.NewOperation pattern used for vic-machine service API

* whitespace

* whitespace

* clean up trace.op pattern in API handler functions

* bug fix: validateTarget takes in context since used by buildData
zjs pushed a commit that referenced this pull request Nov 20, 2017
@AngieCris @zjs
Use trace.NewOperation pattern in vic-machine service API handlers (#…
b4bacb0 
…6563)

* trace.NewOperation pattern used for vic-machine service API

* whitespace

* whitespace

* clean up trace.op pattern in API handler functions

* bug fix: validateTarget takes in context since used by buildData
AngieCris added a commit to AngieCris/vic that referenced this pull request Nov 20, 2017
@AngieCris
Use trace.NewOperation pattern in vic-machine service API handlers (v…
d5f5865 
…mware#6563)

* trace.NewOperation pattern used for vic-machine service API

* whitespace

* whitespace

* clean up trace.op pattern in API handler functions

* bug fix: validateTarget takes in context since used by buildData
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jzt jzt jzt approved these changes

@andrewtchin andrewtchin andrewtchin approved these changes

@mdharamadas1 mdharamadas1 mdharamadas1 approved these changes

@zjs zjs Awaiting requested review from zjs

@emlin emlin Awaiting requested review from emlin

Assignees

@AngieCris AngieCris

Labels
cla-not-required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@AngieCris @jzt @andrewtchin @mdharamadas1 @vmwclabot