feat(opentelemetry): expose functionality as plugin #1884
Conversation
| * | ||
| * When a value is provided, modules will be appended to existing modules. | ||
| */ | ||
| readonly workflowInterceptorModules?: PluginParameter<string[]>; |
There was a problem hiding this comment.
Necessary for the OTEL plugin to function properly when used when prebundling
There was a problem hiding this comment.
Looks reasonable to me, not sure if there are greater implications here @mjameswh
| * | ||
| * @experimental Plugins is an experimental feature; APIs may change without notice. | ||
| */ | ||
| export class OpenTelemetryClientPlugin extends SimplePlugin { |
There was a problem hiding this comment.
I split these into 2 plugins vs making the resource/traceExporter optional to make it easier to use these plugins correctly.
If we wanted to make a stronger type error we could brand the various plugin interfaces and make it clear this plugin isn't valid to be used when creating a worker. At the moment since all plugin methods are optional any object with a name property is a valid plugin for any context. I would be open to branding the interfaces, but I think that deserves its own PR.
There was a problem hiding this comment.
I split these into 2 plugins vs making the resource/traceExporter optional to make it easier to use these plugins correctly.
I agree with this sentiment.
If we wanted to make a stronger type error we could brand the various plugin interfaces and make it clear this plugin isn't valid to be used when creating a worker. At the moment since all plugin methods are optional any object with a name property is a valid plugin for any context. I would be open to branding the interfaces, but I think that deserves its own PR.
Imo, I don't think this is absolutely necessary atm, given that you've separated them.
The overlying concern is relevant to all plugins. I think it's reasonable to create a separate issue to introduce a helper enforcing stronger plugin typing.
| */ | ||
| export class OpenTelemetryWorkerPlugin extends SimplePlugin { | ||
| constructor(readonly otelOptions: OpenTelemetryWorkerPluginOptions) { | ||
| const workflowInterceptorsPath = require.resolve('./workflow-interceptors'); |
There was a problem hiding this comment.
Remind me, is it always the case that we want to register inbound, outbound, and internal workflow interceptors?
There was a problem hiding this comment.
Good catch, I can definitely see people wanting to omit some of those interceptors. I'll provide the functionality via plugin options.
There was a problem hiding this comment.
I added the ability to disable any individual interceptor on the Worker plugin. I skipped this ability for the client plugin as the way to do that is just not use the plugin 🤷
| } from './worker'; | ||
| import { OpenTelemetrySinks } from './workflow'; | ||
|
|
||
| export type OpenTelemetryClientPluginOptions = InterceptorOptions; |
There was a problem hiding this comment.
A bit weird that we have this type to extract it back into InterceptorOptions, but I have no real qualms with it.
I like that it's namespaced. Do we expect this to grow beyond InterceptorOptions in the future?
There was a problem hiding this comment.
Possibly which is why I went with export interface OpenTelemetryClientPluginOptions extends InterceptorOptions {} initially which a lint told me is equivalent to just a type alias. If we end up needing to add additional options here we will do it via extends InterceptorOptions and it will be non-breaking.
| * | ||
| * @experimental Plugins is an experimental feature; APIs may change without notice. | ||
| */ | ||
| export class OpenTelemetryClientPlugin extends SimplePlugin { |
There was a problem hiding this comment.
I split these into 2 plugins vs making the resource/traceExporter optional to make it easier to use these plugins correctly.
I agree with this sentiment.
If we wanted to make a stronger type error we could brand the various plugin interfaces and make it clear this plugin isn't valid to be used when creating a worker. At the moment since all plugin methods are optional any object with a name property is a valid plugin for any context. I would be open to branding the interfaces, but I think that deserves its own PR.
Imo, I don't think this is absolutely necessary atm, given that you've separated them.
The overlying concern is relevant to all plugins. I think it's reasonable to create a separate issue to introduce a helper enforcing stronger plugin typing.
| ], | ||
| }, | ||
| sinks, | ||
| plugins: [new OpenTelemetryWorkerPlugin({ resource: staticResource, traceExporter })], |
| * | ||
| * When a value is provided, modules will be appended to existing modules. | ||
| */ | ||
| readonly workflowInterceptorModules?: PluginParameter<string[]>; |
There was a problem hiding this comment.
Looks reasonable to me, not sure if there are greater implications here @mjameswh
This reverts commit e4cacf3.
chris-olszewski
force-pushed
the
olszewski/otel_plugin
branch
from
b0ea3e0 to
86d97f2
Compare
| return toggle !== false; | ||
| } | ||
|
|
||
| type WorkflowInterceptorModule = |
There was a problem hiding this comment.
I went down the road of generating this union, but it was not easily understood and felt like overkill since we probably won't be adding any new workflow interceptor types.
What was changed
Expose the OpenTelemetry interceptors as 2 plugins to reduce possible misuse of them.
Why?
Correctly setting up OTEL interceptors requires correctly setting multiple configuration options in order for it to work correctly.
Checklist
Closes [Feature Request] Implement OTel interceptors v1 as Plugin #1850
How was this tested:
OTEL sample updated to use plugin. Applicable OTEL tests updated to use plugin instead of setting up interceptors directly. Not all tests applicable as some tests don't require fully configured OTEL setups.
Any docs updates needed?
No. We could advertise the plugin variant in the top level docs, but unsure if we want to wait until plugins are stable.