Skip to content
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

Merged
merged 5 commits into from
May 20, 2020
Merged

Conversation

hmdhk
Copy link
Contributor

@hmdhk hmdhk commented May 4, 2020

This PR adds agent.currentTraceIds to typings.

Fixes #1630

Checklist

  • Implement code
  • Add tests
  • Update TypeScript typings
  • Update documentation
  • Commit message follows commit guidelines

@hmdhk hmdhk requested a review from watson May 4, 2020 13:21
@apmmachine
Copy link
Contributor

apmmachine commented May 4, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Branch indexing]

  • Start Time: 2020-05-19T18:21:02.407+0000

  • Duration: 18 min 28 sec (1107577)

Test stats 🧪

Test Results
Failed 0
Passed 179244
Skipped 0
Total 179244

Copy link
Member

@vigneshshanmugam vigneshshanmugam left a 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

Copy link
Contributor

@watson watson left a 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?

@hmdhk
Copy link
Contributor Author

hmdhk commented May 11, 2020

@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

@hmdhk hmdhk requested a review from watson May 11, 2020 12:18
@watson
Copy link
Contributor

watson commented May 11, 2020

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 😓

Copy link
Contributor

@watson watson left a 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

test/types/index.ts Outdated Show resolved Hide resolved
Co-authored-by: Thomas Watson <w@tson.dk>
@hmdhk hmdhk requested a review from watson May 12, 2020 09:52
@watson
Copy link
Contributor

watson commented May 12, 2020

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 master just to see that everything passes.

test/types/index.ts Outdated Show resolved Hide resolved
Co-authored-by: Thomas Watson <w@tson.dk>
@hmdhk hmdhk requested a review from watson May 15, 2020 11:56
@watson watson merged commit 0d4e623 into elastic:master May 20, 2020
@paulrostorp
Copy link

Any visibility on when this will be released ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Property 'currentTraceIds' does not exist on type 'Agent'
6 participants