-
Notifications
You must be signed in to change notification settings - Fork 227
include gorouter_time metrics report #471
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
Conversation
|
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.
I think there are at least two options forward:
- Move the call to CaptureGorouter to the accesslog handler. And move the calculation to gorouter time to access log handler.
- OR set "FinishedAt" in the reporter handler, calculate gorouter_time in the reporter handler, send a metric for gorouter_time in the reporter handler, set gorouter_time on the reqInfo in the reporter handler, and then use that calculated value for gorouter time in the access log handler.
Also needed
- tests
- this needs to be behind the config flag for per request metrics
I prefer the option #2. nvm. Figured out the stack been called in reverse order while processing the response. So finished at will be called once all the request been processed. |
96ee98a
to
f048622
Compare
I think you might also need to add a promethius metric implementation: https://github.com/cloudfoundry/gorouter/blob/main/metrics_prometheus/metrics.go |
29d2635
to
209f8fa
Compare
209f8fa
to
37c8da3
Compare
|
return -1 | ||
} | ||
} | ||
|
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.
I'm confused how the access log record tests still pass with no updates. After removing gorouterTime(), and having it log the pre-calculated GorouterTime value, the GorouterTime would be 0, unless the test was modified to set GorouterTime on the fake access log record used in the tests.
... right? 😕
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.
Figured this issue after running the whole suite. Addressed them here #473.
@@ -107,7 +107,6 @@ var _ = Describe("AccessLog", func() { | |||
Expect(alr.Request.URL).To(Equal(req.URL)) | |||
Expect(alr.Request.RemoteAddr).To(Equal(req.RemoteAddr)) | |||
Expect(alr.ExtraHeadersToLog).To(Equal(extraHeadersToLog)) | |||
Expect(alr.FinishedAt).ToNot(BeZero()) |
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.
Can you replace this with a check to make sure we get a nonzero value on GorouterTime in the access log record?
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.
Addressed it here. #473
Closing this PR due to CLA issue. Opened a new one. #473 |
Summary
Backward Compatibility
Breaking Change? No