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

Improve rack resource names #285

Merged
merged 2 commits into from
Dec 22, 2017
Merged

Conversation

p-lambert
Copy link
Member

@p-lambert p-lambert commented Dec 20, 2017

This PR is an attempt to provide better resource names for rack.

Currently, rack resource names are <REQUEST_METHOD> <RESPONSE_STATUS>.

We're providing an alternative that should generate more meaningful names. The approach taken here is to return the name of the middleware that effectively generated the response.

Let's say you have an AuthenticationMiddleware. In a Rails application context, an unauthorized request would never hit the Rails stack and it would surface as something like aGET 401 resource name.
By enabling the middleware_names option will you get a AuthenticationMiddleware#GET resource name instead.

Datadog.configure do |c|
  c.use :rack, middleware_names: true
end

Also worth noting here that in case the HTTP request ever "hits" your Sinatra/Rails routing layer you will still get the resource names provided by them (e.g, MailerController#create)

@p-lambert p-lambert added the do-not-merge/WIP Not ready for merge label Dec 20, 2017
@p-lambert p-lambert force-pushed the pedro/improve-rack-resource-names branch from c68a070 to b54a109 Compare December 20, 2017 16:53

def resource_name_for(env, status)
if Datadog.configuration[:rack][:middleware_names]
return "#{env['RESPONSE_MIDDLEWARE']}##{env['REQUEST_METHOD']}"
Copy link
Contributor

@delner delner Dec 20, 2017

Choose a reason for hiding this comment

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

Minor, non-blocking thing: would it be cleaner to do a simple if else instead of a return? Usually I see return only if you need to short-circuit a lot of code quickly (Opting not to use return is usually out of the general concern of return's side effects.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I'll make that change 👍

end

def patched?
return @patched if defined?(@patched)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I follow this... "Return patched if patched defined, otherwise return... patched"? Redundant?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is for having a memoized value that can be false.

Ideally we should have:

def patched?
  @patched ||= false
end

But that obviously doesn't work for falsey values.

@p-lambert p-lambert force-pushed the pedro/improve-rack-resource-names branch 4 times, most recently from 2531c27 to f5a0551 Compare December 20, 2017 22:02
@p-lambert p-lambert force-pushed the pedro/improve-rack-resource-names branch from f5a0551 to f1e0eea Compare December 20, 2017 22:55
@p-lambert p-lambert removed the do-not-merge/WIP Not ready for merge label Dec 20, 2017
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.

👍

@p-lambert p-lambert merged commit 05f69c7 into master Dec 22, 2017
@palazzem palazzem added this to the 0.11.0 milestone Jan 17, 2018
@delner delner deleted the pedro/improve-rack-resource-names branch February 9, 2018 18:52
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.

3 participants