Skip to content

[CFX-6266] feat(telemetry): add TrackWithShared for unified Amplitude property groups#513

Draft
ajalon1 wants to merge 13 commits into
datarobot-oss:mainfrom
ajalon1:feat/telemetry-shared-properties
Draft

[CFX-6266] feat(telemetry): add TrackWithShared for unified Amplitude property groups#513
ajalon1 wants to merge 13 commits into
datarobot-oss:mainfrom
ajalon1:feat/telemetry-shared-properties

Conversation

@ajalon1
Copy link
Copy Markdown
Contributor

@ajalon1 ajalon1 commented May 19, 2026

RATIONALE

Currently, Amplitude scopes event properties to their event type. This means that when task_name is registered as an event property on dr task, dr run, and dr task run separately, Amplitude treats them as three distinct properties — dr task › task_name, dr run › task_name, and dr task run › task_name — rather than a single unified one. Any cross-command question like "which tasks are most commonly run?" requires manually aggregating three separate Amplitude properties, and any filtering or funnel built on task_name silently misses events from the other two commands.

This property-scoping limitation applies to any logical "property group" that spans multiple commands: we have the same problem with plugin_name across install/uninstall/update, and missing_deps across check and install.

This PR introduces TrackWithShared: a registration function identical to TrackWith in its closure-invocation contract (thanks @taras-pokornyy!), but which additionally declares a set of property keys that belong to a unified Amplitude property. Client.Track seeds those keys as `` on all events, and commands that registered a shared extractor override the default via their EventProperties.

CHANGES

Before

flowchart LR
    A[dr task dev] -->|TrackWith| B[task event\ntask_name: dev]
    C[dr run dev] -->|TrackWith| D[run event\ntask_name: dev]
    E[dr auth set-url] --> F[auth event\nno task_name]
    B --> G[Amplitude\ndr task.task_name]
    D --> H[Amplitude\ndr run.task_name]
Loading

After

flowchart LR
    R[TrackWithShared] -->|registers key| S[sharedPropKeys]
    A[dr task dev] -->|shared extractor| B[task event\ntask_name: dev]
    C[dr run dev] -->|shared extractor| D[run event\ntask_name: dev]
    E[dr auth set-url] -->|Client.Track seeds| F[auth event\ntask_name: empty]
    B --> G[Amplitude\ntask_name unified]
    D --> G
    F --> G
Loading
  • Add TrackWithShared(cmd, keys, extract) and backing sync.Maps (internal/telemetry/wire.go) — marks the command as tracked, stores the extractor in sharedExtractors, and records each declared key in sharedPropKeys. The extractor is invoked at cobra.OnFinalize time (after RunE), so closures over local variables populated during command execution work identically to TrackWith.
  • Extract mergeExtractorProps helper (internal/telemetry/wire.go) — refactors the load-and-invoke pattern shared by commandProperties and sharedExtractors into a single function. (Appeases the linter.)
  • Seed shared keys on every event (internal/telemetry/telemetry.go) — Client.Track iterates sharedPropKeys and inserts "" for any key not already in the common-property map before merging event-specific properties.
  • Migrate task_name to TrackWithShared (cmd/task/cmd.go, cmd/task/run/cmd.go) — no change to task events; non-task events now additionally carry task_name: "".
  • Add TODO for CFX-6052 (cmd/templates/setup/cmd.go) — marks template_name, template_id, and template_version for the same migration once that work exposes those values from the TUI model. (I think Vol this is something we can use for template_name, if you haven't already ...)

TESTING

Test What it verifies
TestTrackWithShared_SetsAnnotationAndRegistersKeys Annotation is set on the command and all declared keys land in sharedPropKeys
TestTrackWithShared_PassesPropertiesFromExtractor EventFor invokes the shared extractor and includes its output in EventProperties
TestTrackWithShared_CoexistsWithTrackWith A command with both TrackWith and TrackWithShared produces all properties correctly
TestTrackWithShared_NonRegisteredCommandGetsEmptyKey sharedPropKeys is populated after registration; EventFor does not seed (seeding is Client.Track's responsibility)

Run with task test.

TODO

  • CFX-6052 — migrate template_name, template_id, template_version to TrackWithShared once the template setup TUI exposes those values
  • Follow-up: migrate component_name, plugin_name, missing_deps/wrong_version_deps using the same pattern

NOTES

TrackWithShared is intentionally a separate registration path from TrackWith rather than a flag on the existing function. The distinction matters: per-command properties (e.g. parallel, output_format) should not appear on every event as empty strings — only properties that span a logical group of commands benefit from that treatment.

PR Automation

Comment-Commands: Trigger CI by commenting on the PR:

  • /trigger-smoke-test or /trigger-test-smoke - Run smoke tests
  • /trigger-install-test or /trigger-test-install - Run installation tests

Labels: Apply labels to trigger workflows:

  • run-smoke-tests or go - Run smoke tests on demand (only works for non-forked PRs)

Important

For Forked PRs: The run-smoke-tests label won't work. A required Smoke Tests check will block merge until a maintainer acts:

  • A maintainer uses /approve-smoke-tests to run smoke tests (results will set the check)
  • A maintainer uses /skip-smoke-tests to bypass the check without running tests

Please comment requesting a maintainer review if you need smoke tests to run.

ajalon1 and others added 13 commits May 19, 2026 14:35
…roups

TrackWithShared is like TrackWith but declares keys as 'shared' — keys
that appear on every telemetry event (as "" when not applicable) so
Amplitude sees a single unified property rather than N per-event-type
properties.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Client.Track now iterates sharedPropKeys and ensures each declared
shared key is present in every event's properties (defaulting to "").
Commands that registered a TrackWithShared extractor override these
defaults via their EventProperties, so Amplitude sees one property
across all event types.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Covers four cases:
- annotation is set and keys are registered in sharedPropKeys
- shared extractor output appears in EventFor's EventProperties
- TrackWithShared coexists correctly with TrackWith on the same command
- non-registered commands do not get shared props in EventFor (seeding
  is Client.Track's responsibility, validated via sharedPropKeys state)

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
task_name is now a shared telemetry property so Amplitude sees it as
a single unified property across all event types, not a per-event-type
key. Non-task events get task_name seeded as "".

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Aligns dr run with dr task so both contribute to the same shared
Amplitude property.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…omplexity

EventFor was exceeding cyclop's max cyclomatic complexity of 10. Extract
the sync.Map load-and-invoke pattern into a shared helper so both the
per-command and shared extractor paths stay readable.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
template_name, template_id, and template_version will be added as
shared properties once CFX-6052 exposes them from the TUI model.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…e preference

Changed Client.Track to delete shared properties with empty-string values
instead of seeding them. This applies to both extractors that returned empty
strings (e.g., FirstArg with no args) and keys registered via TrackWithShared
that were not set on a given event.

Amplitude prefers properties to be omitted entirely when not set, rather than
sent with empty values.
…e preference

Changed Client.Track to delete shared properties with empty-string values
instead of seeding them. This applies to both extractors that returned empty
strings (e.g., FirstArg with no args) and keys registered via TrackWithShared
that were not set on a given event.

Amplitude prefers properties to be omitted entirely when not set, rather than
sent with empty values.

Refactored Track method to extract property merging and omission logic into
separate helper functions (mergeCommonProperties and omitEmptySharedProperties)
to reduce nesting complexity and improve testability.

Updated documentation comments for sharedExtractors, sharedPropKeys, TrackWithShared,
and EventFor to reflect the new omission behavior.

Renamed TestTrackWithShared_NonRegisteredCommandGetsEmptyKey to
TestTrackWithShared_NonRegisteredCommandOmitsKey and added two new tests:
- TestTrackWithShared_EmptyExtractorValueOmitsKey
- TestTrackWithShared_NonEmptyValueIsIncluded

Added integration-level tests to telemetry_test.go.
@ajalon1 ajalon1 requested a review from cdevent May 20, 2026 23:17
@ajalon1 ajalon1 changed the title feat(telemetry): add TrackWithShared for unified Amplitude property groups [CFX-6266] feat(telemetry): add TrackWithShared for unified Amplitude property groups May 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant