-
Notifications
You must be signed in to change notification settings - Fork 761
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.
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
Add TraceId and SpanId generator util #35
Conversation
* characters corresponding to 64 bits. | ||
*/ | ||
export function randomSpanId(): string { | ||
return crypto.randomBytes(SPAN_ID_BYTES).toString('hex'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very heavy operation. We used this approach in the past but switched to a pseudo-random generator instead with crypto
only used to generate a random seed for the process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this will make the code a bit more involved, maybe crypto
can be used for now and let's create an issue to make it faster?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling crypto twice imposes much overhead, compared to an approach like I did in OpenCensus: census-instrumentation/opencensus-node#198
Basically, there is a function that can create both trace-id and span-id by just calling crypto once.
The PR also contains some benchmarks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still too slow. There should only be one call to crypto
for the entire process to generate a seed and then it becomes possible to generate IDs using a pseudo-random algorithm. This would be several thousands of percents faster.
A safer alternative could be to generate a cryptographically secure trace ID and a pseudo-random span ID with the trace ID as seed, but I don't think that's warranted (we haven't hit a collision in years using pseudo-random numbers).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there will probably be many different utilities, would it make more sense to have a folder to hold them like common/util/id.ts
? Even better if we can find a name that doesn't involve the word util
since this generally ends up with a lot of random unstructured code.
* limitations under the License. | ||
*/ | ||
|
||
import * as crypto from 'crypto'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did the SIG decide that opentelemetry-core
is Node specific? That's fine I think and matches the OpenCensus approach but wanted to at least make sure we were being clear on that.
I recognize that the browser based tracer is a secondary use case and I'm open to basically re-implementing the core
package in a web-centric and web-optimized way, just by sharing the TypeScript types. But want to make sure we are making an intentional decision and considering the options.
The main alternative seems to be something where the platform-specific parts of the code are abstracted away and keep the core
package platform neutral and then maybe have core-node
and core-web
that would inject the platform-specific implementations of things like random number generation, timestamp collection, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually totally missed that but it's true that crypto
should be extracted to some kind of platform-node
. IMO this should be the case regardless of a shared core VS multiple cores.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main alternative seems to be something where the platform-specific parts of the code are abstracted away and keep the core package platform neutral and then maybe have core-node and core-web that would inject the platform-specific implementations of things like random number generation, timestamp collection, etc.
Thanks for the feedback, makes sense to me. I really don't have experience in handling these use-cases. It would be nice and useful if you can lay the groundwork/foundation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I would move randomBytes
to a platform-node
package that we can later on have a platform-browser
with the same API. Then generate a seed from that.
I would then use a the result of Math.random()
stored in a Uint32Array
for the IDs. This approach is very fast and safe given the truly random seed and the Xorshift128+ algorithm used by Math.random().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about merging it as it is and refactoring the parts out right after?
Just to keep iterating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely. At this point in time I think most comments should not block PRs but instead we can move forward and open issues to not lose track of these conversations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to add a less expensive way to generate the ids, but for now, it's good enough. Will create . an issue for that.
@draffensperger Do you agree on this? issue #47 |
Yes, I think merging it now and refactoring it later to enable different implementations for Node/browser makes sense. |
No description provided.