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] add request queuing option to the Rack middleware #272

Merged
merged 5 commits into from
Apr 13, 2018

Conversation

palazzem
Copy link
Contributor

@palazzem palazzem commented Dec 12, 2017

Overview

Provides the request queuing time from the Rack middleware if a frontend web server or load balancer sets a specific header. This is considered an experimental feature and may have breaking changes in the future.

Usage

  • your web server should set the x-request-start header
  • your Rack integration should be configured with the following options
Datadog.configure do |c|
  # web_service_name is optional and has `web-server` as default
  c.use :rack, request_queuing: true, web_service_name: 'nginx'
end

Supported format

At the time of writing, only nginx headers are supported in the format of t=1512379167.574.

@palazzem palazzem added do-not-merge/WIP Not ready for merge integrations Involves tracing integrations labels Dec 12, 2017
@palazzem palazzem requested a review from p-lambert December 12, 2017 16:00
header = env[REQUEST_START]

if header
# nginx header is in the format "t=1512379167.574"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

for the current testing phase, only nginx is required; I'll iterate over other formats if they're different.

p-lambert
p-lambert previously approved these changes Dec 12, 2017
Copy link
Member

@p-lambert p-lambert 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 to me! Left some comments about code style, but the changes are not necessary, so feel free to skip them!

request_start = Time.at(time_string.to_f)
request_start.utc > now ? nil : request_start
end
rescue StandardError
Copy link
Member

Choose a reason for hiding this comment

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

In ruby you can omit the exception class when it is StandardError

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

def get_request_start(env, now = Time.now.utc)
header = env[REQUEST_START]

if header
Copy link
Member

Choose a reason for hiding this comment

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

I usually prefer early returns than code nested in conditional branches, but that's only a matter of personal style :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@@ -32,19 +40,34 @@ def initialize(app, options = {})
@app = app
end

def compute_queueing_time(env, tracer)
if Datadog.configuration[:rack][:request_queuing]
Copy link
Member

Choose a reason for hiding this comment

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

Same here about the early return comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@palazzem palazzem added this to the 0.11.0 milestone Dec 14, 2017
@palazzem palazzem force-pushed the palazzem/rack-context branch from e9f3fc1 to 12ca7bc Compare December 14, 2017 10:27
@palazzem palazzem changed the base branch from palazzem/rack-context to master December 14, 2017 13:58
@palazzem palazzem force-pushed the palazzem/request-queuing branch 2 times, most recently from d81a371 to 9980106 Compare December 14, 2017 14:11
@palazzem palazzem removed this from the 0.11.0 milestone Jan 11, 2018
@delner delner added this to the 0.11.1 milestone Jan 16, 2018
@palazzem palazzem modified the milestones: 0.11.1, 0.11.2 Jan 24, 2018
@delner
Copy link
Contributor

delner commented Feb 1, 2018

Is this stable enough to merge? Would be cool to close this out if we're happy with it.


Datadog::Tracer.log.debug('[experimental] creating request.enqueuing span')
tracer.trace(
'request.enqueuing',

Choose a reason for hiding this comment

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

Nit: the name doesn't align with our usual informal conventions (request. looks like it comes from request library).
I also guess it has to stay generic between web servers (not Nginx only).
So maybe something like web_server.start_request, http_server.start_request?
I also saw that there is a potential for another X-Queue-Start timing, it would then easily translate into a whatever.queue_start?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

totally agree, we can change that value with one you've suggested!

Copy link
Contributor

Choose a reason for hiding this comment

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

If there's an HTTP header named X-Queue-Start, those usually get tagged as http. I'd suggest adding http, like http.queue_start, http.headers.queue_start or something similar (to indicate its origins)


Datadog.configure do |c|
# web_service_name defaults to `web-server` and is different from Rack `service_name`
c.use :rack, request_queuing: true, web_service_name: nginx

Choose a reason for hiding this comment

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

Nit: request_queuing is a specific mechanism of the front web server, while here we want to more generally integrate with front web servers. Maybe we could have an option key more generic? Examples: instrument_web|http_server, behind_web_server, http_server_enabled, ...
I guess it also depends on what we will want to do once stable (enabled by default?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the option name, yes we can stay more generic. For the future (once it's stable), we can keep it enabled by default so to activate it is enough to change something in the proxy configuration (nginx, HAProxy, whatever).

@palazzem palazzem modified the milestones: 0.11.2, 0.11.3 Feb 2, 2018
@delner delner modified the milestones: 0.11.3, 0.12.0 Feb 5, 2018
@delner delner changed the base branch from master to 0.12-dev February 5, 2018 19:01
@delner delner added the feature Involves a product feature label Feb 5, 2018
@palazzem palazzem modified the milestones: 0.12.0, 0.13.0 Apr 5, 2018
@palazzem
Copy link
Contributor Author

palazzem commented Apr 5, 2018

Rescheduled for 0.13.0 but will be available in a beta release right after the 0.12.0 release.

@delner delner changed the base branch from 0.12-dev to 0.13-dev April 13, 2018 17:33
@delner delner force-pushed the palazzem/request-queuing branch from 29c7c2a to 9f57afe Compare April 13, 2018 19:00
@delner delner removed the do-not-merge/WIP Not ready for merge label Apr 13, 2018

module_function

def get_request_start(env, now = Time.now.utc)
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider moving this to the core library in a future pull request. The behavior here is generic enough, and useful for other integrations.

Copy link
Contributor

@delner delner left a comment

Choose a reason for hiding this comment

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

👍

@delner delner merged commit 026da76 into 0.13-dev Apr 13, 2018
@palazzem palazzem deleted the palazzem/request-queuing branch April 13, 2018 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Involves a product feature integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants