Add AppInsights #196
Add AppInsights #196
Conversation
Charles-Gagnon
requested review from
chlafreniere and
lucyzhang929
as code owners
| @@ -0,0 +1,64 @@ | |||
| // Copyright (c) Microsoft Corporation. All rights reserved. | |||
| // Licensed under the MIT License. See License.txt in the project root for license information. | |||
There was a problem hiding this comment.
ideally, we'd have some tests for this file 😄
src/Telemetry/Telemetry.cs
Outdated
| /// </summary> | ||
| public const string TelemetryOptoutSetting = "AzureFunctionsSqlBindingsTelemetryOptOut"; | ||
|
|
||
| public const string WelcomeMessage = @"SQL Bindings for Azure Functions |
There was a problem hiding this comment.
I believe this should be "Azure SQL binding for Azure Functions"
|
|
||
| private Dictionary<string, double> GetEventMeasures(IDictionary<string, double> measurements) | ||
| { | ||
| var eventMeasurements = new Dictionary<string, double>(this._commonMeasurements); |
There was a problem hiding this comment.
Can we do any of these other solutions instead (to clean up this code)? https://stackoverflow.com/questions/6422091/convert-idictionary-to-dictionary
There was a problem hiding this comment.
What are you suggesting? Because there's two things happening here :
- We create a clone of the commonMeasurements dictionary (we don't want to modify the original)
- We then add all the passed in measurements to that dictionary, overwriting any properties that might exist
We want to create a clone of the dictionary regardless (using the dictionary passed in could be problematic, we can't trust the caller to be doing the right thing there). And as far as I know there isn't a way to create a "combined" dictionary from two separate sources.
Unless you're just saying that the logic is fine you just think we could compress it - in which case maybe? I'd have to play around with it, but that doesn't really seem worth the effort here given how short they are already.
There was a problem hiding this comment.
Oh, I missed line 128 where we're creating a clone of commonMeasurements. I thought this was just cloning in quite a roundabout fashion.
And yeah was mainly around compressing this into fewer lines. It's fine.
|
@Charles-Gagnon hopefully the other PR will help with the build failure 😄 |
* initial * Cleanup * opt out + cleanup * Remove extra stuff * more cleanup * Undo unneeded changes * Add README * Add back in connection string setting * PR comments
Add AppInsights package and get everything initialized for adding telemetry. No events added in this PR - those will be done in follow-up PRs.
Telemetry can be disabled in two ways :
AZUREFUNCTIONS_SQLBINDINGS_TELEMETRY_OPTOUTAzureFunctionsSqlBindingsTelemetryOptOutIf either of these are set to "1", "true" or "yes" then telemetry is disabled and nothing is logged.
We will also show a welcome message on startup (if logging is enabled) for users to see these options :
(Note I disabled IDE0130 for now. It was complaining about the folder structure because of the RootNamespace hack I did when moving project stuff around. I'm going to come back later and see if I can get it fixed for real but for now I want to keep all this stuff in one folder so disabling the rule is the easiest way)