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

Rack: Accept generic X-Request-Start header #832

Merged
merged 8 commits into from
Nov 6, 2019

Conversation

JamesHarker
Copy link
Contributor

This patch makes the t= portion of the X-Request-Start header optional so that the request queuing time can be supported on platforms like Heroku.

This patch make the `t=` portion of the X-Request-Start header optional so that
the request queuing time can be supported on platforms like Heroku.
@JamesHarker JamesHarker requested a review from a team October 7, 2019 17:08
@JamesHarker JamesHarker changed the base branch from master to 0.28-dev October 7, 2019 19:16
@JamesHarker JamesHarker changed the base branch from 0.28-dev to master October 7, 2019 19:16
Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

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

Thank you for this PR, and welcome @JamesHarker!

I noticed that Heroku's documentation mentions that time is represented in milliseconds.

Our current implementation calls Time.at(time_value), which expects time in seconds, which is what nginx uses.

To handle Heroku headers correctly, a conversion has to be made here.

lib/ddtrace/contrib/rack/request_queue.rb Outdated Show resolved Hide resolved
@JamesHarker
Copy link
Contributor Author

I noticed that Heroku's documentation mentions that time is represented in milliseconds.

Our current implementation calls Time.at(time_value), which expects time in seconds, which is what nginx uses.

To handle Heroku headers correctly, a conversion has to be made here.

Apologies, completely overlooked that. I'll update the PR 👍

Add support for Apache as well
@JamesHarker
Copy link
Contributor Author

@marcotc I've updated the PR as requested. I've also added support for Apache as apparently the default there is something different again 🙄

I've added some more tests but I'm not sure if this if this is OTT? Happy to amend if needs be.

I think I've also forked (and have the PR merging into) the wrong branch - would you like me to change this?

Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

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

Great work @JamesHarker!
The apache support is super welcome too, thank you.

I left a few more comments, but it's looking pretty close to :shipit:.

lib/ddtrace/contrib/rack/request_queue.rb Outdated Show resolved Hide resolved
lib/ddtrace/contrib/rack/request_queue.rb Outdated Show resolved Hide resolved
lib/ddtrace/contrib/rack/request_queue.rb Outdated Show resolved Hide resolved
lib/ddtrace/contrib/rack/request_queue.rb Show resolved Hide resolved
@marcotc marcotc added community Was opened by a community member integrations Involves tracing integrations feature Involves a product feature labels Oct 9, 2019
marcotc
marcotc previously approved these changes Oct 31, 2019
Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

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

Thank you very much @JamesHarker for your responsiveness and a solid PR 🎉

@marcotc
Copy link
Member

marcotc commented Oct 31, 2019

Only a few housekeeping things left to do: address Rubocop issues and sync with master branch.

Co-Authored-By: Marco Costa <mmarcottulio@gmail.com>
@JamesHarker
Copy link
Contributor Author

@JamesHarker The fix to the MongoDB test failure you are seeing in your build is in master branch. Sync with master and you are good!

Master merged back in so I think we're good go? 👍

Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

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

All ✅, thank you!

@marcotc marcotc merged commit 346634d into DataDog:master Nov 6, 2019
@JamesHarker JamesHarker deleted the feature/generic-request-queuing branch November 8, 2019 17:33
@delner delner added this to the 0.29.0 milestone Nov 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Was opened by a community member feature Involves a product feature integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants