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

feat: enable code_level_metrics by default #1723

Merged
merged 2 commits into from
Jul 14, 2023

Conversation

bcanzanella
Copy link
Contributor

Description

Enables code level metrics by default

How to Test

Please describe how you have tested these changes. Have you run the code against an example application?
What steps did you take to ensure that the changes are working correctly?

Related Issues

Please include any related issues or pull requests in this section, using the format Closes #<issue number> or Fixes #<issue number> if applicable.

@CLAassistant
Copy link

CLAassistant commented Jul 13, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@bizob2828 bizob2828 left a comment

Choose a reason for hiding this comment

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

Can you also PR the docs website please? it's says defaulting to false currently. Also there are some unit tests now failing because this is enabled and the tests assert span attrs

Copy link
Member

@bizob2828 bizob2828 left a comment

Choose a reason for hiding this comment

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

tests are broken. just run npm run unit and you will see

@planteater
Copy link

@bizob2828 I'll take care of the docs.

'request.parameters.route.biz': 'bang'
'request.parameters.route.biz': 'bang',
'code.filepath':
'/Users/bcanzanella/code/node-newrelic/test/unit/shim/webframework-shim.test.js',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bizob2828 any suggestion for how to make this generic? checking the entire object won't work unless the value for code.filepath is modified. could check the length of keys and check each key/value pair with the code.filepath doing an endsWith check? thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

instead of t.same you could just assert the individual attrs.

const attrs = segment.getAttributes()
t.equal(attrs['request.parameters.route.foo'], 'bar')
t.equal(attrs['request.parameters.route.biz'], 'bang')

I'm ok with that. we have other places asserting CLM data

@@ -618,7 +623,13 @@ test('WebFrameworkShim', function (t) {
const segment = wrapped(req)

t.ok(segment.attributes)
t.same(segment.getAttributes(), {})
t.same(segment.getAttributes(), {
'code.filepath':
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bizob2828 same comment as the other

Copy link
Member

Choose a reason for hiding this comment

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

i would just update this assertion to make sure there are no request parameters.

const attrs = Object.keys(segment.getAttributes())
const requestParams = /request\.parameters.*/
t.notOk(attrs.some((attr) => requestParameters.test(attr)))

@bcanzanella
Copy link
Contributor Author

ready for another look @bizob2828

@codecov
Copy link

codecov bot commented Jul 14, 2023

Codecov Report

Merging #1723 (8d01374) into main (76112a6) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1723   +/-   ##
=======================================
  Coverage   96.87%   96.87%           
=======================================
  Files         200      200           
  Lines       39181    39181           
  Branches       24       24           
=======================================
  Hits        37956    37956           
  Misses       1225     1225           
Flag Coverage Δ
esm-unit-tests-14.x 47.80% <ø> (ø)
esm-unit-tests-16.x 92.11% <ø> (ø)
esm-unit-tests-18.x 92.11% <ø> (ø)
integration-tests-14.x 79.03% <100.00%> (+0.05%) ⬆️
integration-tests-16.x 79.13% <100.00%> (+0.05%) ⬆️
integration-tests-18.x 79.11% <100.00%> (+0.05%) ⬆️
unit-tests-14.x 91.41% <100.00%> (ø)
unit-tests-16.x 91.47% <100.00%> (ø)
unit-tests-18.x 91.45% <100.00%> (ø)
versioned-tests-14.x 75.70% <100.00%> (-0.07%) ⬇️
versioned-tests-16.x 77.02% <100.00%> (-0.05%) ⬇️
versioned-tests-18.x 77.03% <100.00%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lib/config/default.js 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bizob2828 bizob2828 merged commit 0b96de3 into newrelic:main Jul 14, 2023
19 checks passed
@bcanzanella bcanzanella deleted the feature/enable-clm branch July 14, 2023 19:19
@github-actions github-actions bot mentioned this pull request Jul 20, 2023
@menocomp
Copy link

menocomp commented Nov 9, 2023

I am a little bit confused here.
Docs says this feature is off by default but the changes in the code says it is On by default.
Screenshot from 2023-11-09 11-41-19

@bizob2828
Copy link
Member

@bizob2828 I'll take care of the docs.

@planteater looks like the docs were never updated. #1723 (comment)

@bizob2828
Copy link
Member

I am a little bit confused here.

Docs says this feature is off by default but the changes in the code says it is On by default.

Screenshot from 2023-11-09 11-41-19

@menocomp this is a docs issues. It looks it was missed 4 months ago. We will get it fixed.

@planteater
Copy link

Oops! Looks like I updated the following page, but didn't catch that reference. I'll take care of it.

https://docs.newrelic.com/docs/apm/agents/nodejs-agent/installation-configuration/codestream-integration/

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

Successfully merging this pull request may close these issues.

5 participants