Skip to content

feat: creating common telemetry package#410

Open
Sandeepan-Ghosh-0312 wants to merge 1 commit intomainfrom
feat/commonTelemetryPackage
Open

feat: creating common telemetry package#410
Sandeepan-Ghosh-0312 wants to merge 1 commit intomainfrom
feat/commonTelemetryPackage

Conversation

@Sandeepan-Ghosh-0312
Copy link
Copy Markdown
Contributor

@Sandeepan-Ghosh-0312 Sandeepan-Ghosh-0312 commented May 6, 2026

Description

Creating a common package for telemetry

Check consumption in #413 for coded-action-app sdk and ts-sdk

Comment thread packages/telemetry/src/types.ts
Comment thread packages/telemetry/src/types.ts Outdated
Comment thread rollup.config.js Outdated
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 6, 2026

Three issues found this run.

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 6, 2026

Review findings (3 new inline comments)

1. packages/telemetry/src/types.ts:45loggerName is dead code
Declared in TelemetryClientInitOptions and forwarded from the SDK shim, but client.ts never reads it. Vestige of the old OTel LoggerProvider pipeline. Remove from the interface and call site.

2. packages/telemetry/src/types.ts:109_OtelTypeCheck assertion is vacuously true
A extends B | unknown collapses to A extends unknown, which holds for every type. The first element never catches an attribute-shape mismatch. Direction should be flipped to verify our type is a subtype of OTel's.

3. rollup.config.js:50 — Runtime builds lack the @uipath/telemetry alias; build order is unguarded
dtsAliasConfig inlines telemetry types from source for .d.ts builds, but the runtime createPlugins() path uses aliasConfig (no telemetry entry). Rollup falls back to node_modules resolution → workspace symlink → packages/telemetry/dist/index.js. If the telemetry package has not been built first, the SDK build fails silently. No root prebuild step enforces order. The same gap exists in packages/coded-action-apps/rollup.config.js.

Comment thread packages/coded-action-apps/src/coded-action-apps-service.ts Outdated
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 6, 2026

Review finding (1 new inline comment)

packages/coded-action-apps/src/coded-action-apps-service.ts:43@track decorators will always no-op (missing initialization)

Both @track('CodedActionApps.CompleteTask') and @track('CodedActionApps.GetTask') delegate to the shared @uipath/telemetry singleton, but that singleton is never initialized in the CAA package — so every event is silently dropped. The package needs a thin initialization shim (like src/core/telemetry/index.ts in the SDK) that wires in the CAA connection string and identity before the decorators run.

Comment thread packages/telemetry/package.json Outdated
Comment thread packages/telemetry/src/index.ts Outdated
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 6, 2026

Review finding (1 new inline comment)

packages/telemetry/src/index.ts:22-27 — barrel uses export type { ... } instead of export * from './types'

Convention requires barrel files use export * from — the export type form silently drops runtime values (enums, class constructors) if any are added to types.ts in the future. Simple one-line fix: replace the block with export * from './types'.

Comment thread packages/telemetry/src/client.ts
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 6, 2026

Review finding (1 new inline comment)

packages/telemetry/src/client.ts:118 — regex literals defined inside method bodies

extractInstrumentationKey and extractIngestionEndpoint both construct a RegExp on every call. Per CLAUDE.md convention, inline regex literals that don't change between invocations must be lifted to module-level constants.

Comment thread src/core/telemetry/index.ts Outdated
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 6, 2026

Review finding (1 new inline comment)

src/core/telemetry/index.ts:22-26export type { ... } should be export { ... }

The same convention violation previously flagged and fixed in packages/telemetry/src/index.ts is now present in the SDK shim: export type { TelemetryAttributes, TelemetryContext as TelemetryConfig, TrackOptions } uses the type modifier, which silently drops runtime values if any of these ever acquire a concrete shape (class, enum). Use export { ... } instead.

Comment thread packages/telemetry/src/track.ts Outdated
Comment thread packages/telemetry/src/constants.ts Outdated
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 6, 2026

Review findings (2 new inline comments)

1. packages/telemetry/src/track.ts:59track and trackEvent have no unit tests
client.test.ts only covers TelemetryClient. The track decorator has non-trivial logic (condition evaluation as static boolean or callback, fallback name resolution, function-wrapper vs method-decorator paths) that needs its own test file. Per CLAUDE.md, every public method requires success + error scenario coverage.

2. packages/telemetry/src/constants.ts:9 — wrong workflow filename in JSDoc
Comment references .github/workflows/publish-telemetry.yml which doesn't exist. The actual file is .github/workflows/publish.yml (containing a publish-telemetry job).

Comment thread packages/telemetry/src/constants.ts Outdated
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 6, 2026

Review finding (1 new inline comment)

packages/telemetry/src/constants.ts:9 — wrong workflow filename in JSDoc

The module-level comment references .github/workflows/publish-telemetry.yml which doesn't exist. The connection-string injection is the publish-telemetry job inside .github/workflows/publish.yml.

Comment thread src/core/telemetry/constants.ts
Comment thread packages/coded-action-apps/src/telemetry/index.ts Outdated
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 6, 2026

Review findings (2 new inline comments)

1. src/core/telemetry/constants.ts:12 — orphaned attribute-key constants (incomplete refactoring)

The PR deleted src/core/telemetry/client.ts and removed CONNECTION_STRING from constants.ts, but did not remove the eight attribute-key constants (VERSION, SERVICE, CLOUD_ORGANIZATION_NAME, CLOUD_TENANT_NAME, CLOUD_URL, CLOUD_CLIENT_ID, CLOUD_REDIRECT_URI, APP_NAME, UNKNOWN) that were exclusively consumed by client.ts. These are now dead code that the linter will not catch because they are exported. The file's own header comment says "only the SDK's identity lives here" — these constants do not fit that description; the canonical versions now live in @uipath/telemetry.

2. packages/coded-action-apps/src/telemetry/index.ts:6-8 — inaccurate JSDoc claim about src/index.ts

The comment states the module is imported as a side effect from src/index.ts, but src/index.ts does not import ./telemetry at all. Initialization happens only because coded-action-apps-service.ts imports track from ./telemetry. Adding an explicit import './telemetry' to src/index.ts (or correcting the comment) makes the bootstrap contract robust and honest.

Comment thread .github/workflows/publish.yml Fixed
Comment thread packages/telemetry/tests/track.test.ts
Comment thread packages/telemetry/tests/track.test.ts
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 6, 2026

Review findings (2 new inline comments)

Both are in the newly added packages/telemetry/tests/track.test.ts and violate the CLAUDE.md rule: "NEVER use as unknown as type casts — refactor to make types flow naturally."

1. track.test.ts:26fake as unknown as TelemetryClient in makeClient
Root cause: createTrack accepts the full TelemetryClient class even though it only uses track() and getDefaultEventName(). Extracting a minimal ITrackable interface for createTrack's parameter type eliminates the cast and improves the production design.

2. track.test.ts:123condition as unknown as (...args: unknown[]) => boolean
The mock is typed too narrowly ((value: number) => boolean). Typing it as (...args: unknown[]) => (args[0] as number) > 10 matches TrackOptions.condition directly, removing the double cast while keeping the assertion behaviour unchanged.

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 6, 2026

✅ No issues found. Checked for bugs and CLAUDE.md compliance.

@Sandeepan-Ghosh-0312 Sandeepan-Ghosh-0312 force-pushed the feat/commonTelemetryPackage branch from 0cf66f0 to 0707ef5 Compare May 6, 2026 12:29
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 6, 2026

✅ No issues found. Checked for bugs and CLAUDE.md compliance.

1 similar comment
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 6, 2026

✅ No issues found. Checked for bugs and CLAUDE.md compliance.

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 6, 2026

Review update — 2 threads unresolved

The PR was re-reviewed against the current diff. All previously flagged issues were addressed except the two as unknown as casts in packages/telemetry/tests/track.test.ts — both threads were resolved but neither change was made:

1. track.test.ts:26fake as unknown as TelemetryClient in makeClient
The root cause is that createTrack / createTrackEvent accept the full TelemetryClient class even though they only need track() and getDefaultEventName(). The thread asked for a minimal ITrackable interface to be extracted so the stub satisfies the type structurally and the cast is unnecessary. The current code is unchanged — createTrack(client: TelemetryClient) still requires the concrete class.

2. track.test.ts:133condition as unknown as (...args: unknown[]) => boolean
The thread asked for the mock to be typed as vi.fn((...args: unknown[]) => (args[0] as number) > 10), which matches TrackOptions['condition'] directly and removes the double cast. The current code keeps the narrow (value: number) => boolean type with the double cast.

Both threads have been re-opened.

Co-Authored-By: Claude <claude@users.noreply.github.com>
@Sandeepan-Ghosh-0312 Sandeepan-Ghosh-0312 force-pushed the feat/commonTelemetryPackage branch from 106c2f2 to 8d86ddd Compare May 6, 2026 21:06
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented May 6, 2026

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 6, 2026

✅ No issues found. Checked for bugs and CLAUDE.md compliance.

1 similar comment
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 6, 2026

✅ No issues found. Checked for bugs and CLAUDE.md compliance.

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.

2 participants