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.

Sign up for GitHub

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

Jump to bottom

[net/http] Allow overriding service name. #430

Merged
merged 8 commits into from
May 23, 2018
Merged

[net/http] Allow overriding service name. #430

merged 8 commits into from
May 23, 2018

Conversation

pawelchcki
Copy link
Contributor

@pawelchcki pawelchcki commented May 22, 2018 •
edited

As mentioned in #407 our documentation stated that service_name can be overridden for net/http integration. However that was not the case.

In addition the documentation wrongly stated default service_name as http.

This PR adds ability to override service_name and aligns documentation with actual default service name.

@pawelchcki pawelchcki changed the base branch from master to 0.12-dev May 22, 2018 11:53
@pawelchcki pawelchcki changed the title WIP: Add ability to net/http to override service name. Add ability to net/http to override service name. May 22, 2018
@pawelchcki pawelchcki added do-not-merge/WIP Not ready for merge bug Involves a bug integrations Involves tracing integrations labels May 22, 2018
@pawelchcki pawelchcki changed the title Add ability to net/http to override service name. [net/http] Allow overriding override service name. May 22, 2018
@pawelchcki
Copy link
Contributor Author

@delner It looks like only unrelated tests are failing. Other than that this PR is ready for review.

@pawelchcki pawelchcki removed the do-not-merge/WIP Not ready for merge label May 22, 2018
@pawelchcki pawelchcki requested a review from delner May 22, 2018 17:29
delner
delner requested changes May 22, 2018
View reviewed changes
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.

A minor thing and a possible bug, but otherwise this looks good!

docs/GettingStarted.md
@@ -514,8 +514,10 @@ Where `options` is an optional `Hash` that accepts the following parameters:

| Key | Description | Default |
| --- | --- | --- |
| ``service_name`` | Service name used for `http` instrumentation | http |
| ``service_name`` | Service name used for `http` instrumentation | net/http |
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm glad you're making this correction in the documentation, but this coincidentally highlights maybe a poor name here (which includes a /.) Probably outside the scope of this PR, but we should probably revisit what this default name is in the future.

Rakefile Outdated
@@ -232,11 +232,13 @@ task :ci do
sh 'rvm $MRI_VERSIONS --verbose do appraisal contrib rake spec:graphql'
sh 'rvm $MRI_VERSIONS --verbose do appraisal contrib rake spec:racecar'
sh 'rvm $MRI_VERSIONS --verbose do appraisal contrib rake spec:redis'
sh 'rvm $MRI_VERSIONS --verbose do appraisal contrib rake spec:http'
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor, but can you put this in alphabetical order with the other contrib names?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

lib/ddtrace/contrib/http/patcher.rb Outdated
service = Datadog.configuration[:http][:service_name]
tracer = Datadog.configuration[:http][:tracer]

pin = Datadog::Pin.new(service, app: APP, app_type: Datadog::Ext::AppTypes::WEB, tracer: tracer)
Copy link
Contributor

Choose a reason for hiding this comment

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

So the following is a really nuanced thing, related to a bug I stumbled upon in our Sequel integration...

It's possible that an Net::HTTP object is initialized before Datadog is patched/configured, in which case, the object in question will not trace properly. To avoid this scenario, I would suggest refactoring this pin initialization into its own def datadog_pin function, which lazily initializes the pin.

The other benefit you'd get is that you can remove the #initialize patching altogether from this, making the patch less invasive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup lazy initialization seems to work. Thanks for suggesting it, it made the code much cleaner

@pawelchcki
Copy link
Contributor Author

@delner I've addressed your comments, this PR should be ready for round two

@pawelchcki pawelchcki changed the title [net/http] Allow overriding override service name. [net/http] Allow overriding service name. May 23, 2018
delner
delner approved these changes May 23, 2018
View reviewed changes
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.

Love it! Nice work, @pawelchcki ! 💯

delner
delner requested changes May 23, 2018
View reviewed changes
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.

Oh wait, have to fix the tests first. But pending that, I'm happy with this.

@pawelchcki pawelchcki force-pushed the bugfix/net_http_service_name branch from 0e09ccc to f82706a Compare May 23, 2018 14:10
@pawelchcki pawelchcki changed the base branch from 0.12-dev to master May 23, 2018 14:10
delner
delner approved these changes May 23, 2018
View reviewed changes
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.

👍

@pawelchcki pawelchcki merged commit c58f9cd into master May 23, 2018
@pawelchcki pawelchcki deleted the bugfix/net_http_service_name branch May 23, 2018 15:33
@pawelchcki pawelchcki mentioned this pull request May 24, 2018
Closed
@delner delner added this to the 0.12.1 milestone May 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@delner delner delner approved these changes

Assignees
No one assigned
Labels
bug Involves a bug integrations Involves tracing integrations
Projects
None yet
Milestone
0.12.1
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@pawelchcki @delner