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

Bump tap version #779

Merged
merged 13 commits into from
Jul 21, 2021
Merged

Bump tap version #779

merged 13 commits into from
Jul 21, 2021

Conversation

bizob2828
Copy link
Member

@bizob2828 bizob2828 commented Jun 4, 2021

Proposed Release Notes

  • Upgraded tap to v15
  • Upgraded @newrelic/superagent @newrelic/aws-sdk @newrelic/koa @newrelic/native-metrics and @newrelic/test-utilities to the latest

Links

Closes #802

Details

This needs to land before we can re-test #772

@bizob2828 bizob2828 force-pushed the bump-tap-version branch 2 times, most recently from 05edd7b to 82e1c74 Compare June 8, 2021 19:09
@bizob2828 bizob2828 marked this pull request as ready for review July 16, 2021 19:28
@bizob2828 bizob2828 mentioned this pull request Jul 16, 2021
@bizob2828 bizob2828 marked this pull request as draft July 20, 2021 21:04
@bizob2828 bizob2828 marked this pull request as ready for review July 21, 2021 00:03
@bizob2828 bizob2828 force-pushed the bump-tap-version branch 2 times, most recently from 59d1b78 to b57b53d Compare July 21, 2021 19:01
Copy link
Contributor

@carlo-808 carlo-808 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far. Just a few really minor comments. Waiting for rebase to approve.

@@ -177,7 +181,7 @@ tap.test('Inifinite tracing - Connection Handling', (t) => {
})
})

function testSetup(callback, t) {
function testSetup(t, callback) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for updating the signature


done()
})
var hostName = getMetricHostName(agent, params.memcached_host)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ultra super minor, but could have used const here.

@@ -292,6 +283,8 @@ test('built-in http module instrumentation', (t) => {
const segment = transaction.baseSegment
const spanAttributes = segment.attributes.get(DESTINATIONS.SPAN_EVENT)

// TODO: these attrs aren't getting filtered
// not familiar with code so trying to trace where this is supposed to be removed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the x-filtered-out header is getting filtered out through attribute filtering wildcard rules. Looks like the regex test is /^(?:response\.(?:headers\.(?:setCookie|x))|request\.(?:headers\.(?:setCookie|x)))/g for filtering out.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea i forgot to remove TODO after i traced that

Copy link
Contributor

@carlo-808 carlo-808 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tap updated to 15 🎉 with a lot of test updates (nice work)
Node module deps updated 🎉

@carlo-808 carlo-808 merged commit 334cc31 into main Jul 21, 2021
@bizob2828 bizob2828 deleted the bump-tap-version branch July 21, 2021 22:48
@michaelgoin michaelgoin added this to Done in Node v16 Support Jul 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Update agent dependencies for node 16 support
3 participants