Should this not just be in the else block?

Is there a way we can just configure telemetry.New and pass in the enabled flag into the telemetry struct? I ask because this init code is now ~60 lines long, wondering we can push the else condition just into the RunSnapshotter.

Just a thought.

Can we extract this to it's own function, maybe throw some quick unit tests around just this logic? I know you have a test that tests the full flow, but this PR comes down to this conditional which is a bit hard to read.

We can define the full space of input values and test it easily if we extract it to a function. If I am reading the comments correctly, this is the truth table:

| Description                | telemetryEnabled  | telemetryDisabled | Report Telemetry Disabled |
|----------------------------|-------------------|-------------------|---------------------------|
| New deployment             | <null>            | <null>            | No                        |
| Enabled                    | exists @ time = 0 | <null>            | Yes                       |
| Disabled,but never enabled | <null>            | exists @ time = 0 | No                        |
| Enabled after disabled     | exists @ time = 1 | exists @ time = 0 | No                        |
| Disabled after enabled     | exists @ time = 0 | exists @ time = 1 | Yes                       |

If my understanding is correct, then this code is correct as well.

Just a suggestion if we extract this to a function, we can implement the logic in easier to read conditionals.

func shouldReportDisabledTelemetry(/* ... */) {
  if errors.Is(telemetryEnabled, sql.ErrNoRows) {
    return false // Telemetry was never enabled, so do not report
  }
	
  if errors.Is(telemetryDisabledErr, sql.ErrNoRows) {
    return true // Never reported teletry was disabled, so send the report
  }

  return telemetryEnabled.UpdatedAt.After(telemetryDisabled.UpdatedAt)
}

I think I'd prefer it being 1 row as mentioned above it possible

Is there a benefit to making this 2 rows?

If it was just 1 row, with the last state being recorded, would that be sufficient? As in, just store telemetryEnabled = true/false

Then you'd have to merge recordTelemetryEnabled and this function that sends teh final packet.

| Description                            | telemetryEnabled (db) | telemetryEnabled (is) | Report Telemetry Disabled |
|----------------------------------------|-----------------------|-----------------------|---------------------------|
| New deployment                         | <null>                | true                  | No                        |
| New deployment, but disabled           | <null>                | false                 | No                        |
| Telemetry enabled, and still is        | true                  | true                  | No                        |
| Telemetry enabled, but disabled        | true                  | false                 | Yes                       |
| Telemetry was disabled, now is enabled | false                 | true                  | Yes                       |
| Disabled, still disabled               | false                 | false                 | No                        |

If there is some reason the logic that saves the enabled prop and the logic that sends the packet cannot be combined, then this suggestion would not work.

You're right, I think it would work, and it would simplify the logic too. We'd only need to report telemetry disabled when the telemetryEnabled in db switches from true to false. I'll refactor this.

Feels odd to use numbers, but I'm guessing the table column must be a string. strconv has a ParseBool func. Your call though since the value is only used here.