feat: expose ability to publish otel metrics to remote endpoint#1244
feat: expose ability to publish otel metrics to remote endpoint#1244Hweinstock wants to merge 2 commits into
Conversation
Package TarballHow to installnpm install https://github.com/aws/agentcore-cli/releases/download/pr-1244-tarball/aws-agentcore-0.13.1.tgz |
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
Thanks for wiring this up — the helper extraction and tests are nice. One concern that I think should be addressed before merge: the new isTelemetryEnabled duplicates and diverges from the existing resolveTelemetryPreference in src/cli/telemetry/config.ts, which is the source of truth used by the agentcore telemetry command. See inline comment for details.
41eb4ad to
8285a60
Compare
8285a60 to
07bec2a
Compare
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
Looks good overall — the previous comment about reusing resolveTelemetryPreference has been addressed cleanly, and the test refactor away from temp config files is a nice simplification.
One issue worth thinking about before this gets used in practice: the validated endpoint and the OTLP exporter URL construction don't agree on what "the endpoint" means. See inline comment.
Description
Wire up the OtelMetricSink to the telemetry client when AGENTCORE_TELEMETRY_ENDPOINT is set (via env var or global config). This enables publishing CLI metrics to a remote OTLP-compatible endpoint. The endpoint can also be configured via telemetry.endpoint in the global config file.
Note: this is not enabling telemetry! The intent is to allow the team to manually add the endpoint to their config to start collecting some real data, users that do not explicitly edit their config with an endpoint will be unaffected.
Testing
verified metrics are making to to collector when injecting the endpoint. They are currently being dropped due to an issue in validation that has its own fix.
Related Issue
Closes #
Documentation PR
Type of Change
Testing
How have you tested the change?
npm run test:unitandnpm run test:integnpm run typechecknpm run lintsrc/assets/, I rannpm run test:update-snapshotsand committed the updated snapshotsChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.