Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
App insights Telemetry for Commands #409
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
base: main
Are you sure you want to change the base?
App insights Telemetry for Commands #409
Changes from 14 commits
18d692db3ea0c01697b74206e578186253ea8e925ff7d5292b9879f1102b24aad3bf3f42b9e4e6fdb565682ae27be3f2caed1470b826d370File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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
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.
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: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
commandandsub-commanddon't need to be different events.We can have a single
commandevent that has a property likeCommandIdwhose value is<commandname>/<subcommand name>When we are querying the backend data, we can more easily filter based on the
CommandIdproperty 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
CreateContextevent.