Error: Must be valid TraceId instance

[russell-matt]

After upgrading to version 0.18.5 of zipkin-instrumentation-restify and zipkin, I am getting the following error when the request to my server comes in with a traceId: "Error: Must be valid TraceId instance".

I've narrowed the problem down to the following snippet of code in zipkin/tracer/index.js inside the join function

if (!(traceId instanceof TraceId)) {
    throw new Error('Must be valid TraceId instance');
}

This if is always resulting to false, after inspecting the traceid passed to this function, I found the following.

traceId parameter:

TraceId {
  _traceId: 'b7daf2ae1b9e2555',
  _parentId:
   { type: [Getter],
     present: [Getter],
     map: [Function: map],
     ifPresent: [Function: ifPresent],
     flatMap: [Function: flatMap],
     getOrElse: [Function: getOrElse],
     equals: [Function: equals],
     toString: [Function: toString] },
  _spanId: 'b7daf2ae1b9e2555',
  _sampled:
   { type: [Getter],
     present: [Getter],
     map: [Function: map],
     ifPresent: [Function: ifPresent],
     flatMap: [Function: flatMap],
     getOrElse: [Function: getOrElse],
     equals: [Function: equals],
     toString: [Function: toString] },
  _debug: false,
  _shared: false }

traceId instanceof TraceId_1: false
traceId.constructor.name: TraceId
TraceId_1.constructor.name: Function
TraceId.constructor.name: Function

Please note: The usage of TraceId_1 is because that is the output produced after transpilation, and hence is what is being checked in the code when it is actually running.

if (!(traceId instanceof TraceId_1)) {
    throw new Error('Must be valid TraceId instance');
}

This issue only happens if the request is made with the traceid and spanid headers populated.

[ReeceGarratt]

I've got a similar issue. Looks like its the same cause.

Please look into this.

[Cyberclown]

Same, disaster

[codefromthecrypt]

are all of you using typescript? I will look into this now.

…

On Wed, Jul 24, 2019, 11:20 PM Norman ***@***.***> wrote: Same, disaster — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#422?email_source=notifications&email_token=AAAPVVYHLMLA6ZGORNONSVLQBBXNXA5CNFSM4IGRAQTKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2WVZVA#issuecomment-514677972>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAPVV6DW4WUNAA5PBRDZETQBBXNXANCNFSM4IGRAQTA> .

[Cyberclown]

Normal js, es5 if im not mistaken

[russell-matt]

@adriancole No, I'm using plain javascript running on Node version 10.11.0 locally, and alpine-8 on server.

[codefromthecrypt]

#328 introduced this, specifically f665dc3

looking into it and will put a patch up ASAP

[codefromthecrypt]

as I understand it, this is a trace Id but not the exact type, hence always fail. So we need to use duck typing or loosen up, right?

[cbspindler]

Getting similar issue, will keep watching thread

[russell-matt]

as I understand it, this is a trace Id but not the exact type, hence always fail. So we need to use duck typing or loosen up, right?

I'm not entirely sure if duck typing would help. From what I could see the type was what we wanted it to be, but the instanceof check fails. It seemed to be caused because babel transpiles the class down to a function, and instanceof fails because of this.

Loosing up the check should solve it, as long as we're not doing a hard typing check. Thanks for the quick reply and action, will look forward to the patch.

[codefromthecrypt]

_traceId and _spanId are required.. what if we temporarily use this as a type check until a better way is found? ps anyway if you have a better way, please let me know :)

[russell-matt]

_traceId and _spanId are required.. what if we temporarily use this as a type check until a better way is found? ps anyway if you have a better way, please let me know :)

Sounds like it would work. Essentially all we need to do is make sure that what we want to do in the function will not result in a type error, so checking whatever properties we require to do that should be sufficient.

On that note, the error that was being thrown was bubbling all the way up to the top, and being thrown out of the middleware, thus causing the restify server to crash. Is this intended? It seems odd for a middleware to not catch this error and handle gracefully.

[codefromthecrypt]

different topic but yeah, crashing requests is bad.

[codefromthecrypt]

can someone take a look? I can cut a release immediately after #423

[codefromthecrypt]

ps sorry about the TraceIdApocalypse

[codefromthecrypt]

https://github.com/openzipkin/zipkin-js/releases/tag/v0.18.6

[russell-matt]

Change looks like it should resolve our issue. Will test it and let you know.

[russell-matt]

Updated to 0.18.6, tested and everything looks fine. Thanks @adriancole for the fast resolution!

[jcchavezs]

On that note, the error that was being thrown was bubbling all the way up to the top, and being thrown out of the middleware, thus causing the restify server to crash. Is this intended? It seems odd for a middleware to not catch this error and handle gracefully.

@russell-matt do you mind opening an issue for this?

[russell-matt]

@jcchavezs Done, #424

[jcchavezs]

Thank you @russell-matt