allow NewRelicQueryName to be specified for SQL, with unit tests

[kevinpohlmeier]

Description

This discussion suggested that it should be possible to name SQL queries. This is helpful if there are multiple queries that touch the same table, or if queries are only listed as "MSSQL (subquery)".

The feature was not added to the roadmap, but it would really help me. I tested this by adding unit tests, and by deploying the dlls to my local NewRelic instance and verifying that they display properly in the UI. Doing that actually revealed a bug in my original code when my segment contained a slash character.

My proposed changes do not remove any of the text that the code originally calculated. It should work exactly the same as before, unless the SQL text contains a comment of the form /* NewRelicSegmentName: myNewRelicSegmentName */. If that is done, it will add that myNewRelicSegmentName to the name in brackets.

In my particular case, I am using Entity Framework to generate my SQL text. I am intercepting the SQL commands as explained here and adding comments at the top of each query.

Author Checklist

• Unit tests
• Integration tests, and Unbounded tests completed
• Performance testing completed with satisfactory results (if required)
• Agent Changelog or Lambda Agent changelog updated

Reviewer Checklist

• Perform code review
• Pull request was adequately tested (new/existing tests, performance tests)
• Review Changelog

[CLAassistant]

All committers have signed the CLA.

[nr-ahemsath]

Hi, @kevinpohlmeier! Thank you for submitting this PR, and for including unit tests. We should be able to review this before the end of the week.

[nr-ahemsath]

@kevinpohlmeier Just to follow up - I have been reviewing this PR as time as allowed since last week; I've been experimenting with a few different tests and everything looks fine. We have a team design review meeting tomorrow where I plan to demo how your feature works to the rest of the team, and we should have a decision on whether or not to merge this PR after that. Thanks for your patience.

[nr-ahemsath]

Hello, @kevinpohlmeier: the conclusion from our design review meeting is that we do like this feature and want to get it integrated into our agent. There are a few things to take care of first:

1. We think that NewRelicQueryName would be easier for users to understand than NewRelicSegmentName.
2. You may have noticed that one of the PR checks failed. We have a couple of flaky unit tests (unrelated to SQL parsing) which we have been ignoring until X date; there are recent commits on our repo's main branch to re-ignore them. If you update your fork with the latest changes from our repo this should be fixed. This should also resolve the CHANGELOG conflict.
3. Since datastore metrics are covered by an internal agent specifications document that is shared by all NR language agents, we will need to run this by the spec maintainers to see what they have to say.
4. We'll need to figure out how to document this feature for our users.

You can take care of items 1 and 2, and 3 and 4 are on our plate. Thanks again!

[kevinpohlmeier]

Will do, thanks!

[kevinpohlmeier]

OK, I think I've taken care of those 2 items. Let me know if you need anything else.

[nr-ahemsath]

@kevinpohlmeier Hi, just wanted to let you know we haven't forgotten about this. Most people were out for the holiday last week. We're still doing our internal review process. I'll try to provide a more significant update soon. Thanks for your patience.

[nr-ahemsath]

Hello @kevinpohlmeier, part of our review process involved running this new feature past our security team. They're fine with the feature, but they would like to see the string pulled out for the query name run through an HTML encoding method for safety: https://docs.microsoft.com/en-us/dotnet/api/system.web.httputility.htmlencode?view=netframework-4.5

Would you be able to make this additional change, and maybe add another unit test or two to verify that behavior?

[kevinpohlmeier]

@nr-ahemsath I actually did consider adding this originally, but I see the same potential issue from the existing code extracting a table name from the SQL text.

In my opinion, it would be better to set the model raw within the SqlParser and handle the encoding when saving to the Metric table. That would be outside the scope of a change like I am proposing, where I am just extracting a different piece of the SQL text as the name instead of using the table name.

If I add encoding within the SQLParser now and encoding is also added to a more centralized location later, it will end up being double-encoded and will not display correctly.

I'm not familiar enough with the whole NewRelic codebase to know where the most appropriate place is for a centralized encoding. Possibly the DatastoreSegmentData constructor, or possibly even somewhere that is not SQL-specific? I would really prefer to leave that up to the NewRelic security team as a separate work item.

For example, this SQL is valid and poses the same potential security issue when extracting a table name as the Model.

CREATE TABLE [test"><script>alert("I'm malicious")<script>] (x INT)

SELECT * FROM [test"><script>alert("I'm malicious")<script>]

[nr-ahemsath]

@kevinpohlmeier Yeah...pretty much as soon as I hit save on my previous comment, I thought "wait a minute...shouldn't we already be doing this in some central place for ALL strings that come from external sources?" And since I don't have our entire codebase memorized, I wouldn't be surprised if we already do. I'll dig into it soon.

[github-actions]

This PR has been marked stale after 30 days with no activity. It will be closed in 5 days if there is no activity.

[kevinpohlmeier]

@nr-ahemsath I saw the notice about this becoming stale, and wanted to make sure it wasn't forgotten.

Since the security team's concern should be addressed in a more centralized manner, can this change be merged in without that change? Or will it have to wait until the centralized security concerns are addressed?

[angelatan2]

@kevinpohlmeier, Thanks for noticing the stale label by the bot. I have removed the label. @nr-ahemsath will discuss with the team on next steps and we hope to close this soon.

[nr-ahemsath]

@kevinpohlmeier Sorry for the delay in getting this over the finish line. After further investigation and testing, we've concluded that no further changes are necessary on your PR re: the HTML encoding issue. I'm ready to approve and merge this PR. If you could update your fork from our repo to pick up the latest changes and fix the conflict in the CHANGELOG, I can do final testing and approve.

[kevinpohlmeier]

@nr-ahemsath My fork is updated, please continue

[nr-ahemsath]

Approved. Tested latest version of fork locally, unit tests and datastore-related unbounded integration tests pass. 👍

[nr-ahemsath]

Thanks again for your contribution and your patience, @kevinpohlmeier. 👍