fix(sdk-node): pass OTLP HTTP metric config#6666
Conversation
Signed-off-by: cyphercodes <cyphercodes@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6666 +/- ##
=======================================
Coverage 94.85% 94.86%
=======================================
Files 377 377
Lines 12751 12770 +19
Branches 2887 2890 +3
=======================================
+ Hits 12095 12114 +19
Misses 656 656
🚀 New features to boost your workflow:
|
|
|
||
| type OtlpHttpMetricExporterConfigModel = NonNullable< | ||
| PeriodicMetricReaderConfigModel['exporter']['otlp_http'] | ||
| >; |
There was a problem hiding this comment.
nit: This type could be exported from the configuration package and used directly.
| } | ||
|
|
||
| function getMetricExporterTemporalityPreference( | ||
| temporalityPreference: OtlpHttpMetricExporterConfigModel['temporality_preference'] |
There was a problem hiding this comment.
nit: Likewise, we could add an ExporterTemporalityPreferenceConfigModel export from the configuration package.
| ); | ||
| }); | ||
|
|
||
| it('passes OTLP HTTP metric exporter connection options from configuration', async function () { |
There was a problem hiding this comment.
nit: Move this it(...) case to a new describe('getPeriodicMetricReaderFromConfiguration', ... section. If I read this correctly, this is currently in the getBatchLogRecordProcessorConfigFromEnv describe block.
| }); | ||
|
|
||
| assert.ok(reader); | ||
| const exporter = getMetricExporterInternals(reader); |
There was a problem hiding this comment.
Could the same thing be accomplished with a type assertion to any? E.g.:
| const exporter = getMetricExporterInternals(reader); | |
| const exporter = (reader as any)._exporter; |
I guess I'm expressing a personal preference to avoid defining interface FooInterals types that give the veneer of type safety for spelunking into internal object details.
MikeGoldsmith
left a comment
MikeGoldsmith
left a comment
There was a problem hiding this comment.
This looks good to me, thanks @cyphercodes.
A gap I still see, that I tried to resolve in my accidental duplicate PR (#6707), is the gRPC metric exporter only passes compression. It's missing endpoint, timeout, credentials (TLS), and metadata (headers) — same properties you're now passing for HTTP. This matches the pattern in the trace exporter gRPC path (#6705).
I'd be happy to do a follow-up for the gRPC parameters if you'd prefer to keep this PR scoped to HTTP.
|
Hey @cyphercodes do you still plan on working on this PR? |
|
Updated this PR to resolve the merge conflict with current What changed:
Local verification:
|
4 participants
Which problem is this PR solving?
Fixes #6665.
startNodeSDKwas creating OTLP HTTP metric exporters from declarative config with only compression wired through, so settings such as endpoint, headers, timeout, TLS, temporality, and histogram aggregation were ignored.Short description of the changes
headers_listand structuredheaders, with structured headers taking precedence.Type of change
How Has This Been Tested?
cd experimental/packages/opentelemetry-sdk-node && npm run lintcd experimental/packages/opentelemetry-sdk-node && npm run compilecd experimental/packages/opentelemetry-sdk-node && npm test -- --grep "passes OTLP HTTP metric exporter"cd experimental/packages/opentelemetry-sdk-node && npm test(190 passing, 3 pending)npx markdownlint-cli2 experimental/CHANGELOG.mdgit diff --checkChecklist: