-
Notifications
You must be signed in to change notification settings - Fork 224
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
fix: add currentTraceIds to typings #1733
Conversation
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.
@hamid Can you also add the currentTraceIds
to types test? makes it easy to catch errors.
https://github.com/elastic/apm-agent-nodejs/blob/master/test/types/index.ts
e16aae4
to
a32e398
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.
This PR seems to contain more than what it's supposed to... e.g. the changes to nodejs.memory.arrayBuffers.bytes
etc should not be part of this right?
@watson , that's correct, the first commit on the PR is not mine. There seems to have been a force-push to master which probably caused this problem. I will rebase on the new master |
a32e398
to
dc7d4f2
Compare
Ah yes, that force push was me. There was a problem with a commit that was merged into master so I had to force push it to fix tests - sorry about that 😓 |
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.
Looks great! Just a tiny nit
Co-authored-by: Thomas Watson <w@tson.dk>
The Windows error you see on Jenkins is unrelated and is fixed in master. It's just an issue with Docker on Jenkins. I'll merge in |
Co-authored-by: Thomas Watson <w@tson.dk>
Any visibility on when this will be released ? |
This PR adds
agent.currentTraceIds
to typings.Fixes #1630
Checklist
Update documentation