feat(tel): implement otel sink - #1888
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## refactor #1888 +/- ##
============================================
+ Coverage 95.98% 95.99% +0.01%
============================================
Files 208 209 +1
Lines 9684 9743 +59
============================================
+ Hits 9295 9353 +58
- Misses 389 390 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
^ the line missing coverage is |
|
|
||
| async shutdown(): Promise<void> { | ||
| try { | ||
| await this.meterProvider.forceFlush({ timeoutMillis: this.flushTimeoutMs }); |
There was a problem hiding this comment.
outside this try block, there is meterProvider.shutdown() which also flushes the metric again.
https://opentelemetry.io/docs/specs/otel/metrics/sdk/#shutdown
can we ensure one export somehow?
There was a problem hiding this comment.
This method provides a way for provider to do any cleanup required.
Shutdown MUST be called only once for each MeterProvider instance. After the call to Shutdown, subsequent attempts to get a Meter are not allowed. SDKs SHOULD return a valid no-op Meter for these calls, if possible.
Shutdown SHOULD provide a way to let the caller know whether it succeeded, failed or timed out.
Shutdown SHOULD complete or abort within some timeout. Shutdown MAY be implemented as a blocking API or an asynchronous API which notifies the caller via a callback or an event. [OpenTelemetry SDK](https://opentelemetry.io/docs/specs/otel/overview/#sdk) authors MAY decide if they want to make the shutdown timeout configurable.
Shutdown MUST be implemented at least by invoking Shutdown on all registered [MetricReader](https://opentelemetry.io/docs/specs/otel/metrics/sdk/#metricreader) and [MetricExporter](https://opentelemetry.io/docs/specs/otel/metrics/sdk/#metricexporter) instances.
from https://opentelemetry.io/docs/specs/otel/metrics/sdk/#shutdown.
I don't see any explicit lines in the protocol linked for shutdown that it also flushes. Based on some testing, I think it does internally, but I think its safer to make that behavior explicit. If we flush twice, its a no-op anyway.
|
|
||
| if (globalConfig.telemetry.enabled) | ||
| metricSinks.push( | ||
| new OtelHistogramSink({ |
There was a problem hiding this comment.
it seems like a wrong/malformed endpoint makes getMetricSinks() reject, and shutdown() propagates that rejection, erroring out in the CLI command. Is that understanding correct? can we make it best-effort?
There was a problem hiding this comment.
I think we (the dev team) should be the only ones modifying the endpoint for testing purposes. In which case, I think the ideal behavior is that we reject early.
If a user decides to go into the global config and add an invalid override, I think rejecting is reasonable.
Problem
The AgentCore CLI is not currently publishing telemetry to our collector.
Solution
Testing