refactor!(otlp-grpc-exporter-base): remove headers from gRPC config type#6487
Conversation
dyladan
left a comment
There was a problem hiding this comment.
Nothing against the actual code change but the CHANGELOG entry is wrong and package-lock is unnecessarily changed.
| * fix(opentelemetry-instrumentation): improve `_warnOnPreloadedModules` function not to show warning logs when the module is not marked as loaded [#6095](https://github.com/open-telemetry/opentelemetry-js/pull/6095) @rlj1202 | ||
| * fix(sdk-trace-base): derive internal `SpanOptions` from API type to prevent drift [#6478](https://github.com/open-telemetry/opentelemetry-js/pull/6478) @overbalance | ||
| * fix(span): enforce `attributePerEventCountLimit`, `attributePerLinkCountLimit`, `linkCountLimit`, and `attributeValueLengthLimit` for event/link attributes [#6479](https://github.com/open-telemetry/opentelemetry-js/pull/6479) @overbalance | ||
| * fix(otlp-grpc-exporter-base): remove `headers` from gRPC exporter config type, passing headers now results in a compile-time error instead of being silently ignored [#6487](https://github.com/open-telemetry/opentelemetry-js/pull/6487) |
There was a problem hiding this comment.
This is in the wrong place and replaces an existing entry rather than creating a new one. This should be under the :breaking: header in experimental/CHANGELOG.md. It is also not a bugfix so i'd consider switching this to be a refactor! type.
|
@dyladan thanks for the review! |
|
|
||
| * fix(opentelemetry-instrumentation): improve `_warnOnPreloadedModules` function not to show warning logs when the module is not marked as loaded [#6095](https://github.com/open-telemetry/opentelemetry-js/pull/6095) @rlj1202 | ||
| * fix(sdk-trace-base): derive internal `SpanOptions` from API type to prevent drift [#6478](https://github.com/open-telemetry/opentelemetry-js/pull/6478) @overbalance | ||
| * fix(span): enforce `attributePerEventCountLimit`, `attributePerLinkCountLimit`, `linkCountLimit`, and `attributeValueLengthLimit` for event/link attributes [#6479](https://github.com/open-telemetry/opentelemetry-js/pull/6479) @overbalance |
| url?: string; | ||
| concurrencyLimit?: number; | ||
| timeoutMillis?: number; |
There was a problem hiding this comment.
These typically come from OTLPExporterConfigBase. If they're removed here we risk them becoming out of sync. It would be better to remove the bad header property from the base and add it specifically to the HTTP exporters. Alternatively you could use Omit<T, K> to remove it from the parent class before using it here.
There was a problem hiding this comment.
Switched to Omit<OTLPExporterConfigBase, 'headers'> and All tests passed. Pls review it.
There was a problem hiding this comment.
hmm, IMO it's a positive thing that they diverge:
- they're different exporters and the same options (
urlin particular) behave differently based on whether they're used for gRPC or http, which makes them hard to document. - we also don't consume
OTLPGRPCExporterConfigNodeasOTLPExporterConfigBaseanywhere anymore. - it makes it less likely that things are added to the base that unintentionally get added to the gRPC exporter's public API which can confuse users (like what happened to
headersin this case).
There was a problem hiding this comment.
That makes sense! So should I revert back to the original approach (manually listing the fields with TS types) and drop the Omit change?
There was a problem hiding this comment.
We can do that in a follow up PR, your current change addresses what the linked issue was about. :)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6487 +/- ##
=======================================
Coverage 95.73% 95.73%
=======================================
Files 364 364
Lines 11826 11826
Branches 2765 2765
=======================================
Hits 11322 11322
Misses 504 504 🚀 New features to boost your workflow:
|
pichlermarc
left a comment
There was a problem hiding this comment.
I'm okay with the current change % this blocking comment, as the current change addresses the original intent of the linked issue.
We can discuss not having OTLPGRPCExporterConfigNode extend OTLPExporterConfigBase in a follow-up, but it's not necessary to do right now.
|
All issues fixed, ready for re-review! |
Fixes #5520
if you pass
headerswhen using gRPC exporter, it just gets silently ignored. no error, nothing. pretty confusing for users.turns out
OTLPGRPCExporterConfigNodewas extendingOTLPExporterConfigBasewhich hasheadersin it (that's for HTTP only). gRPC doesn't use HTTP headers, it usesmetadatainstead.Short description of the changes
removed
extends OTLPExporterConfigBasefromOTLPGRPCExporterConfigNodemanually added only the fields gRPC actually needs (
url,concurrencyLimit,timeoutMillis)removed the
diag.warnfor headers since TS will catch it at compile time nowall 3 exporters (trace, metrics, logs) import this type from base so they're all fixed with this one change
Type of change
passing
headerswill now be a TS compile error instead of silently doing nothing. i think this is fine since it was never actually working anyway users should usemetadatafor gRPCHow Has This Been Tested?
existing tests for all packages pass:
otlp-grpc-exporter-baseexporter-trace-otlp-grpcopentelemetry-exporter-metrics-otlp-grpcexporter-logs-otlp-grpcChecklist: