fix(telemetry): classify deployment method and runtime environment instead of unknown - #1791
Conversation
…stead of unknown Most OSS telemetry events reported deployment_method and runtime_environment as unknown because both were inferred from env vars that shipped artifacts never set. - Bake NODE_ENV=production into the runner image so Docker deployments stop reporting runtime_environment=unknown (dev stage deliberately untouched: it runs npm install at startup, which would skip devDependencies under production) - Rework detectDeploymentMethod() into a precedence chain: platform-injected vars (adds Render, Fly, Cloud Run, ECS, Coolify) > INSFORGE_DEPLOYMENT_METHOD artifact stamp > legacy POSTGRES_HOST heuristic > /.dockerenv > source - Stamp INSFORGE_DEPLOYMENT_METHOD in the Dockerfile (docker) and compose files (docker-compose, dokploy) - Add unit tests covering the precedence chain Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WalkthroughChangesDeployment method telemetry
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Summary
Solid, well-scoped telemetry fix that replaces env-var sniffing with an artifact-stamp precedence chain and bakes NODE_ENV=production into the runner image; the code, tests, and backward-compat handling all hold up under review.
Requirements context
No matching spec/plan found — InsForge/InsForge has no /docs/superpowers/ or /docs/specs/ directory (docs are product/deployment docs under docs/). Assessed against the PR description and verified against the actual code paths in the workspace.
Findings
Critical
(none)
Suggestion
-
Software engineering / test coverage —
backend/tests/unit/telemetry.service.test.ts:244-296: The five new tests lock in the precedence order well (platform var > stamp > heuristic > dockerenv > source), but onlyrailwayexercises the platform-var tier. The five newly-added platform branches —render,fly,cloud-run,ecs,coolify(telemetry.service.ts:298-316) — have no test asserting each env var maps to its expected value. A typo in any of those key names (e.g.K_SERVICE,ECS_CONTAINER_METADATA_URI_V4) would ship silently. A smallit.eachover[{env, expected}]for the platform tier would close the gap cheaply. -
Functionality —
backend/src/services/telemetry/telemetry.service.ts:325-328: The stamp istrim().toLowerCase().slice(0,32)with no allowlist, so a user who editsINSFORGE_DEPLOYMENT_METHODin a compose file mints an arbitrary new cardinality value (bounded only by 32 chars, not by a known set). The PR already acknowledges this tradeoff and the length cap is a reasonable mitigation; flagging only so the dashboard side is aware distinct values aren't enumerable.
Information
-
Functionality / observability —
Dockerfile:120+backend/src/utils/logger.ts:14-20,62-65: Verified theNODE_ENV=productionaudit is accurate. The winston stack-suppression gate only affects rawErrorobjects; the primary error path (backend/src/api/middlewares/error.ts:13) logserr.stackas a plain string field, which is unaffected — so self-hosted dashboard error logs keep their stacks. The only behavioral delta is ad-hoclogger.error('…', { error: <Error instance> })calls losing the stack, which is the documented, intended security gate. No regression. -
backend/src/api/middlewares/rate-limiters.ts:421: Confirmed theNODE_ENV !== 'test'guard is unchanged by baking production (production !== 'test'matches the prior unset-env behavior). The dev stage is correctly left untouched. -
Minor: the PR body states
isDevelopment/isProduction"have no callers" — they have no production callers, butbackend/tests/unit/environment.test.tsdoes reference them. Immaterial to the audit's conclusion. -
Security: No new user input reaches SQL/shell/HTTP; env vars are read-only classification signals. No secrets/PII newly logged or returned. No auth changes. No new dependencies. Setting
NODE_ENV=productionis a net security improvement (stack suppression). No concerns. -
Performance: The added
fs.existsSync('/.dockerenv')runs only oninstance_startedand the 24h heartbeat — not a hot path. No N+1, no blocking work of note.
Verdict
approved (informational — a human still gives the explicit GitHub approval). No Critical findings; two non-blocking Suggestions worth a look, primarily the per-platform test coverage. Precedence chain, Dokploy bug fix (stamp=dokploy beats the POSTGRES_HOST=postgres heuristic — verified in docker-compose.dokploy.yml:64,75), and backward-compat for pre-stamp artifacts all verified correct.
Greptile SummaryThis PR fixes telemetry classification for
Confidence Score: 5/5Safe to merge — all changes are telemetry-only with no API-visible behavior changes, and the new detection logic is well-tested. The precedence chain in detectDeploymentMethod() is straightforward and fully covered by the new test suite. Baking NODE_ENV=production into the runner image is standard practice, and the PR carefully documents the one behavioral side-effect (winston stack suppression for raw Error objects). Dead code in environment.ts and a missing length-cap test are the only gaps, neither of which affects runtime behavior. No files require special attention. backend/src/utils/environment.ts has two unused helpers that were advertised as removed but were not included in the diff. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[detectDeploymentMethod called] --> B{Platform env var set?}
B -- Yes --> C[Return platform name]
B -- No --> D{INSFORGE_DEPLOYMENT_METHOD set?}
D -- Yes --> E[trim + lowercase + slice 0..32]
D -- No --> F{/.dockerenv exists?}
F -- Yes --> G[Return 'docker']
F -- No --> H[Return 'source']
Reviews (3): Last reviewed commit: "test(telemetry): cover each platform det..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/tests/unit/telemetry.service.test.ts (2)
244-296: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct cases for each newly supported platform.
The matrix only exercises Railway among platform variables. Add cases for Render, Fly, Cloud Run, ECS, Coolify, and Kubernetes so a wrong environment key or return value cannot regress unnoticed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/unit/telemetry.service.test.ts` around lines 244 - 296, Add direct parameterized test cases to the deployment-method matrix in the deployment method test, covering Render, Fly, Cloud Run, ECS, Coolify, and Kubernetes. Each case should set the platform-specific environment variable(s) recognized by the telemetry detection logic and assert the corresponding deployment_method value, matching the existing Railway case structure.
56-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse constant naming for the environment-key list.
Rename
deploymentEnvKeystoDEPLOYMENT_ENV_KEYS.As per coding guidelines, use UPPER_CASE for constants and Enum members.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/unit/telemetry.service.test.ts` around lines 56 - 75, Rename the module-level constant deploymentEnvKeys to DEPLOYMENT_ENV_KEYS and update its reference in clearDeploymentEnvironment. Preserve the existing environment-key contents and cleanup behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/services/telemetry/telemetry.service.ts`:
- Around line 325-328: Update the deployment-method stamping logic around the
INSFORGE_DEPLOYMENT_METHOD value to emit only the allow-listed channels docker,
docker-compose, and dokploy. Return the normalized value only when it matches
one of these values; otherwise continue into the existing heuristics and
fallback instead of truncating and emitting arbitrary input.
---
Nitpick comments:
In `@backend/tests/unit/telemetry.service.test.ts`:
- Around line 244-296: Add direct parameterized test cases to the
deployment-method matrix in the deployment method test, covering Render, Fly,
Cloud Run, ECS, Coolify, and Kubernetes. Each case should set the
platform-specific environment variable(s) recognized by the telemetry detection
logic and assert the corresponding deployment_method value, matching the
existing Railway case structure.
- Around line 56-75: Rename the module-level constant deploymentEnvKeys to
DEPLOYMENT_ENV_KEYS and update its reference in clearDeploymentEnvironment.
Preserve the existing environment-key contents and cleanup behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 36c6ef0d-4cfc-4513-b20e-fcba98c81583
📒 Files selected for processing (6)
Dockerfilebackend/src/services/telemetry/telemetry.service.tsbackend/tests/unit/telemetry.service.test.tsdocker-compose.dokploy.ymldocker-compose.prod.ymldocker-compose.yml
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 6 files
Confidence score: 4/5
- In
backend/src/services/telemetry/telemetry.service.ts, allowing arbitraryINSFORGE_DEPLOYMENT_METHODvalues can still explodedeployment_methodcardinality, fragmenting telemetry and undermining trend/report accuracy against the bounded-classification goal — constrain this to an allowlist (or map unknowns to a stable fallback likeother).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/src/services/telemetry/telemetry.service.ts">
<violation number="1" location="backend/src/services/telemetry/telemetry.service.ts:326">
P2: Arbitrary `INSFORGE_DEPLOYMENT_METHOD` values are emitted as telemetry dimensions, so a user-provided unique value still fragments `deployment_method` despite the stated bounded/stable classification goal. Restrict this artifact stamp to the three supported channel values, then fall through to the legacy detection chain for anything else.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (stamped) { | ||
| return stamped.slice(0, 32); | ||
| } |
There was a problem hiding this comment.
P2: Arbitrary INSFORGE_DEPLOYMENT_METHOD values are emitted as telemetry dimensions, so a user-provided unique value still fragments deployment_method despite the stated bounded/stable classification goal. Restrict this artifact stamp to the three supported channel values, then fall through to the legacy detection chain for anything else.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/src/services/telemetry/telemetry.service.ts, line 326:
<comment>Arbitrary `INSFORGE_DEPLOYMENT_METHOD` values are emitted as telemetry dimensions, so a user-provided unique value still fragments `deployment_method` despite the stated bounded/stable classification goal. Restrict this artifact stamp to the three supported channel values, then fall through to the legacy detection chain for anything else.</comment>
<file context>
@@ -292,13 +295,47 @@ function detectDeploymentMethod(): string {
+ // Dockerfile, PaaS templates) declares its channel. Length-capped to keep
+ // property cardinality bounded when users edit the value.
+ const stamped = process.env.INSFORGE_DEPLOYMENT_METHOD?.trim().toLowerCase();
+ if (stamped) {
+ return stamped.slice(0, 32);
+ }
</file context>
| if (stamped) { | |
| return stamped.slice(0, 32); | |
| } | |
| if (stamped === 'docker' || stamped === 'docker-compose' || stamped === 'dokploy') { | |
| return stamped; | |
| } |
…helpers - Remove the POSTGRES_HOST === 'postgres' heuristic: the new detector only ships in images built from our Dockerfile, which always carry the baked INSFORGE_DEPLOYMENT_METHOD stamp that outranks it, so the branch is unreachable in every artifact we ship - Remove the DOKPLOY_PROJECT_NAME check: Dokploy passes env vars via .env/compose interpolation and does not inject into containers; the stamped dokploy compose identifies that channel - Drop the ECS v3 metadata var; V4 is universal since ECS agent 1.39.0 - Remove unused isDevelopment()/isProduction() helpers and their tests Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
backend/src/services/telemetry/telemetry.service.ts (1)
318-325: 🔒 Security & Privacy | 🟡 MinorAllow-list artifact-stamp values before emitting telemetry.
Truncation limits length, not cardinality: arbitrary environment values are still emitted and could expose accidental secrets or other sensitive data. Accept only the stamped channels (
docker,docker-compose, anddokploy); otherwise continue to the Docker/source fallback. This is the same unresolved issue raised in the previous review.Proposed fix
const stamped = process.env.INSFORGE_DEPLOYMENT_METHOD?.trim().toLowerCase(); - if (stamped) { - return stamped.slice(0, 32); + if (stamped && ['docker', 'docker-compose', 'dokploy'].includes(stamped)) { + return stamped; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/services/telemetry/telemetry.service.ts` around lines 318 - 325, Update the artifact-stamp handling around INSFORGE_DEPLOYMENT_METHOD to return a value only when it matches the allowed channels docker, docker-compose, or dokploy; otherwise continue into the existing Docker/source fallback instead of truncating and emitting arbitrary input.
🧹 Nitpick comments (1)
backend/tests/unit/environment.test.ts (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover
getApiBaseUrl()in this suite.The new helper is neither imported nor tested, so regressions in
API_BASE_URLhandling or thehttp://localhost:7130fallback can pass unnoticed. Add tests for both an explicit value and the unset-environment fallback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/unit/environment.test.ts` at line 1, Update the environment test suite to import and cover getApiBaseUrl, adding cases that verify an explicit API_BASE_URL is returned and that an unset API_BASE_URL falls back to http://localhost:7130.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@backend/src/services/telemetry/telemetry.service.ts`:
- Around line 318-325: Update the artifact-stamp handling around
INSFORGE_DEPLOYMENT_METHOD to return a value only when it matches the allowed
channels docker, docker-compose, or dokploy; otherwise continue into the
existing Docker/source fallback instead of truncating and emitting arbitrary
input.
---
Nitpick comments:
In `@backend/tests/unit/environment.test.ts`:
- Line 1: Update the environment test suite to import and cover getApiBaseUrl,
adding cases that verify an explicit API_BASE_URL is returned and that an unset
API_BASE_URL falls back to http://localhost:7130.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74f9c646-3536-44c3-8da0-08a59feb0689
📒 Files selected for processing (4)
backend/src/services/telemetry/telemetry.service.tsbackend/src/utils/environment.tsbackend/tests/unit/environment.test.tsbackend/tests/unit/telemetry.service.test.ts
💤 Files with no reviewable changes (1)
- backend/src/utils/environment.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/unit/telemetry.service.test.ts
…elpers Per review: every platform env var now has a direct test case so a typo in a variable name cannot ship silently. Restore isDevelopment/ isProduction and their tests — removing them was unrelated to this PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Problem
After a month of OSS telemetry,
deployment_methodandruntime_environmentare dominated byunknown. Both were inferred from environment variables that our shipped artifacts never set:NODE_ENV, so every Docker-based deployment reportedruntime_environment=unknown.detectDeploymentMethod()only recognized four platforms plus aPOSTGRES_HOST === 'postgres'heuristic. Plaindocker run, compose with an external Postgres, unrecognized PaaS platforms, and running from source all fell through tounknown. Dokploy users reporteddocker-composebecause the Dokploy compose hardcodesPOSTGRES_HOST=postgresand Dokploy never injectsDOKPLOY_PROJECT_NAMEinto containers.Changes
Runtime environment
NODE_ENV=productioninto the runner stage. Audited every consumer: winston stops attaching stacks for rawErrorobjects (its documented security gate; the error middleware logs stacks as plain strings and is unaffected), the rate-limiter!== 'test'guard is unchanged, and HTTP responses are byte-identical. The dev stage is deliberately untouched — it runsnpm installat container startup, which would skip devDependencies under production.Deployment method — replaced sniffing with a precedence chain, following the artifact-stamping pattern used by Grafana, PostHog, and n8n:
INSFORGE_DEPLOYMENT_METHODstamp:dockerbaked into the runner image,docker-composein the compose files,dokployin the Dokploy compose (Dokploy injects nothing, so the stamp is what identifies it). One image for all channels; each config artifact overrides with one line. Values are trimmed, lowercased, and length-capped to bound property cardinality./.dockerenv→dockerfor images built before the stamp.source(running from a clone) instead ofunknown.Cleanup — removed paths that are unreachable or dead:
POSTGRES_HOST === 'postgres'heuristic: the new detector only ships in images built from this Dockerfile, which always carry the baked stamp that outranks it.DOKPLOY_PROJECT_NAMEcheck (never injected — see above) and the ECS v3 metadata var (V4 is universal since ECS agent 1.39.0).Tests
docker compose configvalidates all three compose files;docker build --checkclean.v2.2.8-oss-telemetry): passing run — no fixture changes needed, the diff is telemetry-only with no API-visible behavior.Dashboard note
unknowndisappears fromdeployment_methodafter this release — the bucket drains intodocker/sourceand the platform names. Segment byversionwhen comparing trends across the boundary.🤖 Generated with Claude Code