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.

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

fix: audit timers of same name should accumulate (#1435) #1443

Merged
merged 1 commit into from Aug 15, 2017

Conversation

lrowe
Copy link
Contributor

@lrowe lrowe commented Aug 10, 2017

Pre-Submission Checklist

  • Opened an issue discussing these changes before opening the PR
  • Ran the linter and tests via make prepush
  • Included comprehensive and convincing tests for changes

Issues

Closes:

Audit log timers should accumulate their value when startHandlerTimer/endHandlerTimer is called more than once for the same timer name.

Changes

Accumulates audit log timers by timer name.

@yunong
Copy link
Member

yunong commented Aug 10, 2017

@lrowe thanks for the submission. How does this affect a handler chain with multiple handlers with the same name? In this case, we want each of the identically named handlers to have separate timers, which I believe is the current behaviour.

Copy link
Member

@yunong yunong left a comment

Choose a reason for hiding this comment

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

@lrowe thanks for the submission. How does this affect a handler chain with multiple handlers with the same name? In this case, we want each of the identically named handlers to have separate timers, which I believe is the current behaviour.

@yunong yunong merged commit a2d34aa into restify:5.x Aug 15, 2017
@DonutEspresso
Copy link
Member

Q: is this technically a breaking change?

@lrowe
Copy link
Contributor Author

lrowe commented Aug 16, 2017

This isn't breaking change since the API remains compatible. I don't think it even meets the bar for a feature since there's no additional functionality exposed.

No code will break as a result of this change. The only thing existing users might see is a discontinuity in metrics.

@DonutEspresso
Copy link
Member

That's true, but audit log output has now changed, so any tooling that was consuming req.timers in the logs will now be seeing something different. While the field name itself hasn't changed, the method of calculating that output has changed (i.e., behavioral change) - I could see an argument both ways - or am I off my rocker?

@lrowe
Copy link
Contributor Author

lrowe commented Aug 17, 2017

It is a behavioural change, but I really do consider the previous behaviour of overwriting same named timings errant. Pragmatically, I don't expect Restify consumers would need to change any code as a result this fix, so bracketing its release in with breaking changes doesn't seem worthwhile.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants