revert: rollback "chore: add is_container_env to telemetry MCP-2 #330
Conversation
This reverts commit 96c8f62.
fmenezes
changed the title
gagik
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR reverts the previous addition of the is_container_env telemetry property and related container-detection logic, restoring the original telemetry behavior.
- Remove
is_container_envfrom common properties and types - Strip out container environment detection and associated filesystem checks
- Update telemetry initialization and emission flow to use an async startup and catch promise rejections
- Adjust unit and integration tests to reflect the removal of container logic and new async patterns
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/telemetry/types.ts | Removed is_container_env from CommonProperties |
| src/telemetry/telemetry.ts | Dropped container detection, refactored startup/emit flow |
| src/tools/tool.ts | Made emitToolEvent async and added .catch() |
| src/server.ts | Added .catch() to emitEvents call |
| src/logger.ts | Removed unused telemetryContainerEnvFailure log ID |
| tests/unit/telemetry.test.ts | Updated imports, removed container test helpers, await emitEvents |
| tests/integration/telemetry.test.ts | New integration test for actual machine ID resolution |
Comments suppressed due to low confidence (1)
tests/unit/telemetry.test.ts:141
- We currently lack a test covering the error path when
apiClient.sendEventsfails. Adding a test to simulate a send error and verify that events are buffered and retried would improve coverage.
await telemetry.emitEvents([testEvent]);
| @@ -74,12 +74,12 @@ export abstract class ToolBase { | |||
| logger.debug(LogId.toolExecute, "tool", `Executing tool ${this.name}`); | |||
|
|
|||
| const result = await this.execute(...args); | |||
| this.emitToolEvent(startTime, result, ...args); | |||
| await this.emitToolEvent(startTime, result, ...args).catch(() => {}); | |||
There was a problem hiding this comment.
[nitpick] Silently swallowing errors may make debugging failures difficult. Consider logging the caught error or at least adding a comment explaining why it's safe to ignore.
Copilot uses AI. Check for mistakes.
| @@ -130,7 +130,7 @@ export class Server { | |||
| } | |||
| } | |||
|
|
|||
| this.telemetry.emitEvents([event]); | |||
| this.telemetry.emitEvents([event]).catch(() => {}); | |||
There was a problem hiding this comment.
[nitpick] This empty .catch() will swallow any errors from emitEvents. It may be worth logging an error or warning to help diagnose unexpected telemetry failures.
| this.telemetry.emitEvents([event]).catch(() => {}); | |
| this.telemetry.emitEvents([event]).catch((err) => { | |
| logger.warn(`Failed to emit telemetry event: ${err.message}`, { error: err }); | |
| }); |
Copilot uses AI. Check for mistakes.
| "telemetry", | ||
| `Error sending event to client: ${result.error instanceof Error ? result.error.message : String(result.error)}` | ||
| ); | ||
| this.eventCache.appendEvents(events); |
There was a problem hiding this comment.
[nitpick] On send failure the code appends events back to the cache but does not schedule an immediate retry. Consider documenting that retry happens on the next emit or add an explicit retry trigger to avoid long delays in flushing backlog.
Copilot uses AI. Check for mistakes.
Pull Request Test Coverage Report for Build 15979273150Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Reverts #298
fixes #320