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.

Sign up for GitHub

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

Jump to bottom

Adding a flag to enable/disable collection of SQL Command text #1514

Merged
merged 6 commits into from
Feb 18, 2020
Merged

Adding a flag to enable/disable collection of SQL Command text #1514

merged 6 commits into from
Feb 18, 2020

Conversation

stebet
Copy link
Contributor

@stebet stebet commented Dec 20, 2019

Recreating this PR since it wasn't completed before the repo move: microsoft/ApplicationInsights-dotnet-server#1285

Description of original PR

Adding a boolean flag to the DependencyTrackingTelemetryModule which instructs the SQL processors to enable/disable collecting SQL command text.

Also added tests to verify the functionality.

Perhaps there is a better place for that flag than on the module itself since it's propagated down to the other classes, but it made sense to put it there.

This relates to https://github.com/microsoft/ApplicationInsights-aspnetcore/issues/980 as well as
#1282

  • I ran Unit Tests locally.

@TimothyMothra
Copy link
Member

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@TimothyMothra
Copy link
Member

Thank you @stebet for being patient with us while we go through our re-organization.

@lmolkova @cijothomas , it looks like you both informally approved this PR back in October. We had some internal questions about supportability, but I don't remember where we landed on this.

@Dmitry-Matveev
Copy link
Member

Happy New Year!
@lmolkova, do you have a context on the progress and possible merge of this PR? I see your name has been menioned before in this PR.
@SergeyKanzhelev, any concerns, not sure if you were aware of this possible change?

@lmolkova
Copy link
Member

lmolkova commented Jan 9, 2020

I am OK with the PR (allowing users to control), however disabling SQL commands by default is a breaking change as they are collected by default today. Do we require major version update to do behavior changes?

@Dmitry-Matveev
Copy link
Member

I found my comment on the matter in the old PR:

Looks good. Regarding defaults, can you please check with Dave? It may very well be a privacy requirement to have it opt-in only anyway (reqs recently were updated for several privacy considerations), in this case, yes, we're fixing a bug.

So, if it's a privacy bug that we collect SQL command text by default and a bug was there with the original release, technically the major version update is not required. However, there is a debatable point I came across multiple times in the community that long-standing bugs are "features" and fixing them changes the behavior people learned to expect from the software, so it still requires major version update. We can make major version decision when we learn how serious the privacy aspect is. I email Dave CC'ing us.

@stebet
Copy link
Contributor Author

stebet commented Jan 14, 2020

Is collecting SQL commands really a privacy/security issue though? It's up to the users to make sure they aren't putting private stuff in there, and from a security perspective I can't really see how a SQL command text could be considered sensitive. However, from a "opt-in" perspective I'd totally understand it being off by default. Maybe @blowdart has some input into this?

@blowdart
Copy link

It can be yes, we prefer it to be disabled by default. People still built SQL command strings, despite the risk of SQL injection.

@TimothyMothra TimothyMothra added this to the 2.14 milestone Feb 6, 2020
@TimothyMothra
Copy link
Member

TimothyMothra commented Feb 6, 2020 •
edited
Loading

@stebet Thank you again for your patience.
Can you please merge with our develop branch and i'll re-run the tests and give you feedback.

Everybody on the team wants to merge this.
The only hold up is to agree on how to communicate that this is a change in behavior. @Dawgfan I'll sync with you offline.

The plan is to take this in 2.14-beta1 (before the end of Feb). That will give us a little over a month to review the impact before releasing it in the 2.14 stable release (ETA early April).

@stebet
Copy link
Contributor Author

stebet commented Feb 6, 2020

Will do.

@TimothyMothra
Copy link
Member

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@TimothyMothra
Copy link
Member

/AzurePipelines run AI-DotNetSDK-Gated+REORG(Everything)

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@TimothyMothra
Copy link
Member

I'm going to close and re-open this PR to reset the build results.

@TimothyMothra TimothyMothra reopened this Feb 7, 2020
@TimothyMothra
Copy link
Member

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@TimothyMothra
Copy link
Member

@stebet My apologies for all the build agent spam. It appears that one of our build machines got stuck and it was failing builds.

Anyways... I'm working to get the build results public. Can you please review and let me know if you're able to see the build errors?
Several SQL tests are failing, likely due to the behavior change.

@stebet
Copy link
Contributor Author

stebet commented Feb 8, 2020

I'll review and make changes if necessary. Might not happen until on Monday though.

Fixing commandText population to not just work for Stored Procs.
7be064b 
Fixing inconsistent CultureInfo used when conerting double to string and vice versa.
@stebet
Copy link
Contributor Author

stebet commented Feb 10, 2020

  • Fixed a bug in the command text population logic.
  • Also included a fix for inconsistent usage of CultureInfo for doubles in custom metrics, which surfaced when running the tests on a machine using a culture that uses commas instead of periods for number fractions (105,7 instead of the US standard 105.7 for example).

@stebet
Copy link
Contributor Author

stebet commented Feb 10, 2020

/AzurePipelines run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 1514 in repo microsoft/ApplicationInsights-dotnet

@stebet
Copy link
Contributor Author

stebet commented Feb 10, 2020

@TimothyMothra Can you kick off the pipeline tests again?

@TimothyMothra
Copy link
Member

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@stebet
Copy link
Contributor Author

stebet commented Feb 10, 2020

Weird, I don't see these tests in the project. Will take a closer look tomorrow.

@TimothyMothra
Copy link
Member

The Functional Tests are in a different set of projects in the "dotnet\WEB\Test" directory.
You may need docker installed to run these tests locally.
These aren't the most straight forward tests, so please let me know if you get stuck with these.

@stebet
Copy link
Contributor Author

stebet commented Feb 11, 2020

I'm working on those :) will update tomorrow.

@stebet
Copy link
Contributor Author

stebet commented Feb 12, 2020

If @TimothyMothra, @Dmitry-Matveev or @lmolkova could kick off another pipelines build, that'd be great :) Think I have fixed the E2E tests (by enabling SQL Command Text gathering since it's opt-in now).

@TimothyMothra
Copy link
Member

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@stebet
Copy link
Contributor Author

stebet commented Feb 12, 2020

Seems like they timed out but still ran successfully? Can't see any obvious failures.

@TimothyMothra
Copy link
Member

Yeah, I'm looking at it. Seems like Azure DevOps might be having issues today, it won't let me rerun the failed job. Seeing the same behavior on another build I'm working on.

I can re-queue that task, but I want to give them a couple of hours to resolve whatever issues they're having today before I try again.

@TimothyMothra
Copy link
Member

/AzurePipelines run AI_WebSDK_FunctionalTests+Reorg2

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@stebet
Copy link
Contributor Author

stebet commented Feb 14, 2020

@TimothyMothra can we rerun the tests or are there still problems when running them?

@stebet
Copy link
Contributor Author

stebet commented Feb 18, 2020

Probably needs another forced test run...

@TimothyMothra
Copy link
Member

TimothyMothra commented Feb 18, 2020 •
edited
Loading

Sorry for the interruption I was prepping the 2.13.0 stable release last Friday.
I'm going to get your PR merged either today or tomorrow for 2.14-Beta1

@TimothyMothra
Copy link
Member

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@TimothyMothra TimothyMothra merged commit 0e71977 into microsoft:develop Feb 18, 2020
@TimothyMothra
Copy link
Member

@stebet I know this was a long process, but I want to share an emphatic THANK YOU for your patience and willingness to work with us :)

@TimothyMothra TimothyMothra mentioned this pull request Feb 18, 2020
Closed
@TimothyMothra
Copy link
Member

#1690

@stebet stebet deleted the addFlagToDisableSqlCommandText branch February 19, 2020 11:24
@stebet
Copy link
Contributor Author

stebet commented Feb 19, 2020

The pleasure was all mine @TimothyMothra. Completely understand with the repo move and stuff being improved, things can take time. Just happy being able to contribute :)

@cijothomas cijothomas mentioned this pull request Apr 7, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimothyMothra TimothyMothra TimothyMothra approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.14
Development

Successfully merging this pull request may close these issues.
5 participants
@stebet @TimothyMothra @Dmitry-Matveev @lmolkova @blowdart