Skip to content

trace requests per route #189

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
Merged
merged 7 commits into from
Dec 4, 2020
Merged

trace requests per route #189

merged 7 commits into from
Dec 4, 2020

Conversation

rybit
Copy link
Member

@rybit rybit commented Oct 1, 2020 •
edited
Loading

Essentially when I was looking at the tracing in cert-store (and others) all requests for the different routes are under 1 header ("GET" or "PUT") which make it really hard to figure out what's going on. This is an attempt to break that up into per pattern routes.

I figured that I'd have to ditch the middleware approach because it doesn't have the context to know what path it is. If we use the path from the request itself, we're going to get a huge number and they won't aggregate. Sooo this is my compromise.

It is a breaking change in that it changes the behaviour. I added the TraceAllRequests to the router's middleware if people want to upgrade but not change functionality. I will have to test that out though. It is also breaking in the API in that the NewTracer and WrapWithSpan now take a resource. I searched GH in the org and couldn't find any references to those methods outside of commons.

My plan is to:

  • integrate this branch with cert-store and see if I get the trace shape I want
  • test to see if I can maintain behaviour with a few lines
  • discuss and see if this is something we want to adopt

If anyone sees a way that is less hackey to get this to work please let me know.

@rybit rybit added the type: chore work needed to keep the product and development running smoothly label Oct 1, 2020
@rybit
Copy link
Member Author

rybit commented Oct 2, 2020 •
edited
Loading

ok I tested this out with cert-store and it did what I wanted. Take a look here.

image

The thing that is "fixed" is that the endpoints are now not just GET or PUT. They're now method.endpoint. This is so that you can have multiple endpoints with different HTTP methods but the same path and they show up as different endpoints.

I am going to check the behavior to make sure that I can do the other way

@rybit rybit marked this pull request as ready for review October 2, 2020 21:49
@rybit rybit requested a review from a team as a code owner October 2, 2020 21:49
@rybit rybit requested review from smoya, mheffner and mraerino October 2, 2020 21:49
mheffner
mheffner previously approved these changes Oct 5, 2020
View reviewed changes
Copy link
Contributor

@mheffner mheffner left a comment

Choose a reason for hiding this comment

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

Nice improvement!

How about splitting the method from the resource name a bit more clearly? From bitballoon's APM, it looks like they maintain case, so maybe some variant of: GET-resourcename, GET resourcename, GET::resourcename

@rybit
Copy link
Member Author

rybit commented Oct 5, 2020

@mheffner -- yeah that works for me. I went with the :: because I liked it. And I forced to UPPER so that anyone using the method function on the router would be consistent.

mheffner
mheffner previously approved these changes Oct 5, 2020
View reviewed changes
Copy link
Contributor

@mheffner mheffner left a comment

Choose a reason for hiding this comment

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

💃

mheffner
mheffner previously approved these changes Oct 7, 2020
View reviewed changes
Copy link
Contributor

@mheffner mheffner left a comment

Choose a reason for hiding this comment

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

Was the LD change required for this or just trying to make it less noisy?

@rybit
Copy link
Member Author

rybit commented Oct 7, 2020 via email

@rybit rybit requested a review from mheffner December 4, 2020 23:23
mheffner
mheffner approved these changes Dec 4, 2020
View reviewed changes
Copy link
Contributor

@mheffner mheffner left a comment

Choose a reason for hiding this comment

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

looks good 💃

router/errors.go
}
default:
// this is 5ns slower than using unsafe but a unhandled internal server error should not happen that often
if reflect.ValueOf(err).IsNil() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does moving this up to the top add latency we should be concerned about?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so. Also in the error cases I'm generally less worried about latency, everything is screwed already 😂

@rybit rybit merged commit 034c09d into master Dec 4, 2020
@rybit rybit deleted the fix-tracing branch December 4, 2020 23:40
@mraerino mraerino mentioned this pull request Dec 15, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smoya smoya Awaiting requested review from smoya

@mraerino mraerino Awaiting requested review from mraerino

1 more reviewer

@mheffner mheffner mheffner approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
type: chore work needed to keep the product and development running smoothly
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rybit @mheffner