Sketch out key-value logging #53
Conversation
|
(I added a simple usage example) @bcronin please take a look (before I flesh this out) |
| * @param {string} eventName - string associated with the log record | ||
| * @param {object} [payload] - arbitrary payload object associated with the | ||
| * log record. | ||
| * DEPRECATED | ||
| */ | ||
| logEvent(eventName, payload) { | ||
| // Debug-only runtime checks on the arguments |
There was a problem hiding this comment.
Should if DEBUG branch warn about the deprecation? Something like...
if (process.env.NODE_ENV === 'debug') {
console.warn('logEvent has been deprecated. Use log() instead.');
if (arguments.length >= 1 && arguments.length <= 2) {
...There was a problem hiding this comment.
IMO no, but it's just MO :)
|
Makes sense to me 👍 |
|
@bensigelman LGTM, can we merge this? |
|
@yurishkuro sure... in general I was waiting on these PRs until the dust had settled in the various languages. I guess that's happened. Merging now... |
|
Actually, I want to briefly check to make sure the testing stuff that's recently merged is up-to-date. |
|
The testing harness is most likely checking the old |
|
@yurishkuro @bcronin @oibe I added some stuff in commit 5f43e5c |
| * | ||
| * span.log({ | ||
| * "error.description": error.description(), // numeric value | ||
| * }, error.timestampMillis()); |
There was a problem hiding this comment.
This is/was just an imaginary example object... and I'm annoyed that I just merged this since the preceding line has an incorrect "numeric value" comment. I'll fix that in master.
|
LGTM |
|
Alright, I'm going to merge this. Hopefully not controversial at this point. |
Per opentracing/opentracing.io#96
This is only a sketch... once we decide on a specific API I will adjust tests and so on.