Add initial telelmetry skeleton by apoorvdeshmukh · Pull Request #495 · microsoft/go-sqlcmd

apoorvdeshmukh · 2024-01-02T15:27:03Z

Telemetry and logging based on env variable

shueybubbles · 2024-02-27T15:29:02Z

go.mod

 require (
 	github.com/alecthomas/chroma/v2 v2.5.0
 	github.com/billgraziano/dpapi v0.4.0
+	github.com/denisbrodbeck/machineid v1.0.1


machineid v1.0.1

should have a NOTICE update too

shueybubbles · 2024-02-27T15:29:27Z

cmd/modern/root/config/add-context.go

+	c.LogTelemtry()
+}
+
+func (c *AddContext) LogTelemtry() {


emtry(

Telemetry

shueybubbles · 2024-02-27T15:32:19Z

cmd/modern/root/config/add-context.go

 		{localizer.Sprintf("To start interactive query session"), "sqlcmd query"},
 		{localizer.Sprintf("To run a query"), "sqlcmd query \"SELECT @@version\""},
 	}, localizer.Sprintf("Current Context '%v'", context.Name))
+	c.LogTelemtry()


c.LogTelemtry()

instead of requiring run implementation to call this directly, is it possible to define an interface with LogTelemetry as a member and have whatever calls run also call it if the object implements it?

Ideally the command would just populate the event and not be responsible for calling any telemetry package methods directly. We should avoid coupling the command to the telemetry implementation.

shueybubbles · 2024-02-27T15:41:15Z

cmd/modern/root/config/add-context.go

+	properties["SubCommand"] = "add-context"
+	if c.name != "" {
+		properties["name"] = "set"
+	}


This function looks mostly like boilerplate that could be implemented in some common place and just have the command implementation pass the values not the property names for Command and SubCommand.

It's probably also useful to define a new type like

type CommandEvent map[string]string func NewCommandEvent(command string, subCommand string) *CommandEvent { // Set Command and SubCommand properties on new CommandEvent and return the event } func (e *CommandEvent) SetProperty(p string, v string) *CommandEvent { // Add the given property to the map and return e }

shueybubbles · 2024-02-27T16:32:24Z

cmd/modern/root/config/add-user.go

+	eventName := "config-add-user"
+	properties := map[string]string{}
+	if c.username != "" {
+		properties["username"] = "set"


is it easier to build telemetry reports if a property always has a value instead of omitting it when not used? What do the report queries look like?

shueybubbles · 2024-02-27T16:35:03Z

DefineCommand(...CommandOptions)

the telemetry property keys to identify the command and subcommand should be extractable from CommandOptions, correct? instead of having them explicitly added by the command implementation later on.

Refers to: internal/cmdparser/interface.go:27 in 12039f3. [](commit_id = 12039f3, deletion_comment = False)

shueybubbles · 2024-02-27T16:35:44Z

internal/cmdparser/interface.go

+
+	// LogTelemtry is used to track the useful telemetry for the command
+	// To be enforced after the review
+	// LogTelemtry()


// LogTelemtry()

if we switch to a new method, it should probably pass the map as a parameter and not require the command to invoke telemetry methods directly.

shueybubbles · 2024-02-27T16:36:36Z

internal/telemetry/appinsights.go

+)
+
+var telemetryClient appinsights.TelemetryClient
+var isTelemetryEnabled string = "true"


d string = "true"

it's odd to use a string as a bool

shueybubbles · 2024-02-27T16:38:09Z

cmd/modern/root/config/add-context.go

+}
+
+func (c *AddContext) LogTelemtry() {
+	eventName := "config-add-context"


eventName := "config-add-context"

How does this eventName differ from the event name constants defined in internal/telemetry/appinsights.go?

shueybubbles · 2024-02-27T16:40:32Z

internal/telemetry/appinsights.go

+	LocLang   = "Sqlcmd.locLang"
+	UserOs    = "Sqlcmd.userOs"
+
+	// Legacy Flags


// Legacy Flags

putting these as constants implies maintenance overhead every time we add a new flag.

Is there any reason not to build these identifiers dynamically based on the cobra data? In which scenario will an engineer have to type these constant symbols into a code editor?

shueybubbles · 2024-02-27T16:41:38Z

internal/telemetry/appinsights.go

+}
+
+func InitializeAppInsights() {
+	instrumentationKey := "f305b208-557d-4fba-bf06-25345c4dfdbc"


"f305b208-557d-4fba-bf06-25345c4dfdbc"

can we parameterize this key at build time?
It may be desirable to have one key for local builds and a separate production key for our signed builds. The production key won't be in the repo

shueybubbles · 2024-02-27T16:45:02Z

internal/telemetry/appinsights.go

+	}
+}
+
+func CloseTelemetryAndExit(exitcode int) {


Clos

Let's consider registering for os signals instead of requiring an explicit call to CloseTelemetry.
Or is it too late to call CloseTelemetry by the time such a signal arrives?

https://www.developer.com/languages/os-signals-go/#:~:text=Since%20the%20Go%20signal%20notification%20sends%20os.Signal%20values,given%20channel.%20It%20then%20relays%20the%20incoming%20signal.

shueybubbles · 2024-02-27T16:46:22Z

internal/telemetry/userid_windows.go

+	defer k.Close()
+
+	s, _, err := k.GetStringValue("UserId")
+	if err != nil && strings.Contains(err.Error(), "The system cannot find the file specified") {


"The system cannot find the file specified") {

this won't work on non-English Windows

shueybubbles · 2024-02-27T16:47:04Z

internal/telemetry/userid_windows.go

+		s, _, _ = k.GetStringValue("UserId")
+	}
+	if strings.Contains(s, "{") {
+		return s[1 : len(s)-1]


return

s =

Add initial telelmetry skeleton

b566825

This comment was marked as resolved.

Sign in to view

apoorvdeshmukh added 6 commits January 12, 2024 11:53

Merge remote-tracking branch 'origin/main' into apoorvdeshmukh/telemetry

d76a24b

Merge remote-tracking branch 'origin/main' into apoorvdeshmukh/telemetry

d422ad6

Generate persistent unique userId for all events

49972d3

Merge remote-tracking branch 'origin/main' into apoorvdeshmukh/telemetry

ee01884

Merge remote-tracking branch 'origin/main' into apoorvdeshmukh/telemetry

cd4a093

Add event names and properties for telemetry

12039f3

shueybubbles reviewed Feb 27, 2024

View reviewed changes

cmd/modern/root/config/add-context.go

c.LogTelemtry()

}

func (c *AddContext) LogTelemtry() {

Copy link

Collaborator

shueybubbles Feb 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

emtry(

Telemetry

shueybubbles reviewed Feb 27, 2024

View reviewed changes

apoorvdeshmukh closed this Jul 17, 2025

Conversation

apoorvdeshmukh commented Jan 2, 2024

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shueybubbles Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shueybubbles Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shueybubbles commented Feb 27, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shueybubbles Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shueybubbles Feb 27, 2024 •

edited

Loading

shueybubbles Feb 27, 2024 •

edited

Loading

shueybubbles Feb 27, 2024 •

edited

Loading