Skip to content
This repository has been archived by the owner on Oct 3, 2023. It is now read-only.

Use options.host instead of options.hostname (#416) #417

Merged
merged 1 commit into from
Mar 13, 2019

Conversation

jinmel
Copy link
Contributor

@jinmel jinmel commented Mar 12, 2019

Fixes #416

@@ -377,8 +377,8 @@ export class HttpPlugin extends BasePlugin {
const tags = new TagMap();
tags.set(stats.HTTP_CLIENT_METHOD, {value: method});

if (options.hostname) {
Copy link
Member

Choose a reason for hiding this comment

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

I think hostname is preferred over host. I would suggest to add below line.
const host = options.hostname || options.host || 'localhost'; WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

const host = options.hostname || options.host || 'localhost';
span.addAttribute(HttpPlugin.ATTRIBUTE_HTTP_HOST, host);

This change will also fix failing build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks!

Copy link
Contributor Author

@jinmel jinmel left a comment

Choose a reason for hiding this comment

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

I amended the change because it seems like this repository uses rebase policy.
I force pushed to squash it to single commit. Is this correct? I heard this is what you do at google.

@@ -377,8 +377,8 @@ export class HttpPlugin extends BasePlugin {
const tags = new TagMap();
tags.set(stats.HTTP_CLIENT_METHOD, {value: method});

if (options.hostname) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks!

@codecov-io
Copy link

Codecov Report

Merging #417 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #417      +/-   ##
=========================================
+ Coverage   94.79%   94.8%   +<.01%     
=========================================
  Files         136     136              
  Lines        8999    8997       -2     
  Branches      665     663       -2     
=========================================
- Hits         8531    8530       -1     
+ Misses        468     467       -1
Impacted Files Coverage Δ
src/stackdriver-cloudtrace-utils.ts 97.26% <0%> (-0.04%) ⬇️
src/http.ts 91.21% <0%> (ø) ⬆️
test/test-stackdriver-monitoring.ts 95.31% <0%> (ø) ⬆️
src/stackdriver-monitoring-utils.ts
test/test-stackdriver-monitoring-utils.ts
test/test-stackdriver-stats-utils.ts 100% <0%> (ø)
src/stackdriver-stats-utils.ts 99.02% <0%> (ø)
src/stackdriver-monitoring.ts 81.05% <0%> (+0.84%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c0d0b5...caf09be. Read the comment docs.

@mayurkale22 mayurkale22 merged commit 66ebc78 into census-instrumentation:master Mar 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants