-
Notifications
You must be signed in to change notification settings - Fork 238
feat: distributed tracing #538
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
Conversation
5c28cc2 to
fe39c08
Compare
eb08831 to
8d08556
Compare
|
Rebased with #557 to get tests passing. |
6c0b200 to
4892168
Compare
4fd6de6 to
011f5d9
Compare
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.
You forgot to add the traceId to the transaction (and error?) payload if I remember the API correctly.
lib/instrumentation/span.js
Outdated
|
|
||
| Object.defineProperty(this, 'id', { | ||
| get () { | ||
| return this.context.id.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.
It's probably best to cache this value. Currently it's not used more than once, but I wouldn't be surprised if we started to output this in logs as well.
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.
Should probably actually just log the context object, in the future, so the trace id is there too. 🤔
011f5d9 to
f0dedbc
Compare
|
@watson I made some changes to try having a shared buffer and string getters. |
4a6a1a1 to
8fd0cb9
Compare
bd05282 to
24d5548
Compare
|
@Qard the merge conflicts look a little bit complicated, so I didn't want to spend too much time on it. I assume you could do it very quickly 😃 |
|
@Qard never mind. I managed to fix the merge conflicts |
Retargetting #524 to api-v2...
Fixes #421