Skip to content
This repository has been archived by the owner on Sep 13, 2022. It is now read-only.

Log only span context #153

Merged
merged 2 commits into from Sep 27, 2017
Merged

Log only span context #153

merged 2 commits into from Sep 27, 2017

Conversation

yurishkuro
Copy link
Member

@yurishkuro yurishkuro commented Sep 27, 2017

Resolves #123

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.242% when pulling c5dc88b on fix-logging-reporter into 51e40ec on master.

@yurishkuro
Copy link
Member Author

@tiffon @saminzadeh could you please take a look at the build failures? All seem to be related to the error below, a transitive dependency of request module. There was a recent change there request/request#2751. How can we fix it if we're using babel?

> mocha --compilers js:babel-register crossdock/test
/home/travis/build/uber/jaeger-client-node/node_modules/request/node_modules/hawk/node_modules/boom/lib/index.js:5
const Hoek = require('hoek');
^^^^^
SyntaxError: Use of const in strict mode.
    at Module._compile (module.js:439:25)
    at Module._extensions..js (module.js:474:10)
    at Object.require.extensions.(anonymous function) [as .js] (/home/travis/build/uber/jaeger-client-node/node_modules/babel-register/lib/node.js:152:7)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Module.require (module.js:364:17)
    at require (module.js:380:17)

@tiffon
Copy link
Member

tiffon commented Sep 27, 2017

@yurishkuro Looks related to running node 0.10.x which doesn't support const without a flag. Details.

I would say we can either file a bug with request because their upgrade is a breaking change or change our package.json to get rid of the ^ in "request": "^2.74.0". Generally, I would lean toward the latter or both.

@@ -30,7 +30,7 @@ export default class LoggingReporter {
}

report(span: Span): void {
this._logger.info(`Reporting span ${JSON.stringify(span)}`);
this._logger.info(`Reporting span ${span.context().toString()}`);
Copy link
Member

Choose a reason for hiding this comment

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

Interpolated values are automatically converted to strings:

`${{toString: () => 'abc'}}`
// -> "abc"

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't follow. IIRC Javascript does not have Java's convention of automatically calling toString() method when converting to strings.

Copy link
Member

Choose a reason for hiding this comment

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

JS does call toString() when converting to string.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/toString

Every object has a toString() method that is automatically called when the object is to be represented as a text value or when an object is referred to in a manner in which a string is expected.

@yurishkuro
Copy link
Member Author

Looks related to running node 0.10.x which doesn't support const without a flag.

The request module declares "node": ">= 4" in its package.json, so they are free to say they do not intend to support 0.10.

It seems like we're only using request in the crossdock tests, couldn't we use something else that is 0.10 compatible?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.242% when pulling 6553b55 on fix-logging-reporter into 51e40ec on master.

Copy link
Member

@tiffon tiffon left a comment

Choose a reason for hiding this comment

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

The .toString() call is unnecessary, but its a minor issue.

@yurishkuro yurishkuro merged commit 5bdda39 into master Sep 27, 2017
@saminzadeh
Copy link
Member

@yurishkuro Can't we just build the src code with Node 4 (or above) and pipe it through babel so its compatible with Node 0.10?

@yurishkuro
Copy link
Member Author

@saminzadeh we can, but it makes the build a lot more complicated because we want the unit and integration tests run on the actual Node version from Travis matrix. Besides, this request module is a runtime dependency of the crossdock tests, so I don't believe what you propose would've helped with the const issue.

@yurishkuro yurishkuro deleted the fix-logging-reporter branch September 27, 2017 20:16
Iuriy-Budnikov pushed a commit to agile-pm/jaeger-client-node that referenced this pull request Sep 25, 2021
* fix: minor fix

* fix: remove _ prefix from params
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants