-
Notifications
You must be signed in to change notification settings - Fork 63
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
App insights Telemetry for Commands #409
base: main
Are you sure you want to change the base?
Conversation
|
||
// TrackCommand tracks a command execution event | ||
func TrackCommand(command string) { | ||
event := appinsights.NewEventTelemetry("command") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a hierarchy/categorized name for events and properties so we can group things?
Eg in SSMS there are multiple events that happen to an object explorer node. The event names are /
delimited like:
sql/ssms/explorerhierarchynode/setupmenuparent
DataModel.Action.DurationInMilliseconds = 0
SQL.SSMS.ObjectExplorer.Entity = Server/Login
SQL.SSMS.ObjectExplorer.IsSqlDW = False
SQL.SSMS.ObjectExplorer.ServerType = SqlAzureDatabase
sql/ssms/explorerhierarchynode/getmenuitems
DataModel.Action.DurationInMilliseconds = 567.6678
SQL.SSMS.ObjectExplorer.Entity = Server/Login
SQL.SSMS.ObjectExplorer.IsSqlDW = False
SQL.SSMS.ObjectExplorer.ServerType = SqlAzureDatabase
There are also a bunch of "ambient" properties we tried to include in most events, like the type of server OE is connected to.
In our case, I feel like command
and sub-command
don't need to be different events.
We can have a single command
event that has a property like CommandId
whose value is <commandname>/<subcommand name>
When we are querying the backend data, we can more easily filter based on the CommandId
property of one event instead of searching for separate events and manually aggregating them.
Also - for "modern" commands we should log the context id (a guid) with every relevant event. That will enable us to track the lifecycle of a context. EG if the customer runs sqlcmd to create a context, there should be an event that logs the context id guid along with non-PII metadata about the context (container type, operating system of the host, version of SQL, etc). Subsequent commands using that context can then be correlated with that creation.
If the user uses an existing context for an operation, we can log a 'ConnectContext' event that has the similar metadata as the CreateContext
event.
@@ -0,0 +1,67 @@ | |||
package telemetry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it could be useful to define a method that accepts event name and properties plus a func() as input. It would run the function with a timer and add an elapsed time property to the telemetry event before posting it.
Something like
TrackTimedOperation(name string, properties map[string]string, func ())
then we can measure how long it takes to deploy things without trying to look for a "begin" event and its corresponding "end" event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good suggestion , but i would like to add one POV here . Instead of every granular event to have start and end time , we should have these func parmater with specified events (such as deploy and create).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I don't think every operation needs an elapsed time property, just certain ones. At the lowest level, though, the function just needs to take an event name and properties. The application-level code will decide which events need to use it.
These basic method definitions are good for a POC. |
Added a Telemetry package with App Insight resource (the infra would change and so the instrumentation key)
We have added for following commands :
Create
Query
Open
Config
It also capture if sqlcmd run from : legacy /modern.
Following is the snapshot of example commands triggered through sqlcmd and gettig logged in App Insight resource: