Skip to content

chore: Deprecate ActivityInboundLogInterceptor #1284

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Nov 15, 2023

Conversation

mjameswh
Copy link
Contributor

@mjameswh mjameswh commented Nov 8, 2023

What changed

  • Attributes from the current Activity context are now automatically included as metadata on every log entries by the Activity Worker, and some key events of the Activity's lifecycle will automatically be logged (at 'DEBUG' level for most messages; 'WARN' for failures). Logged Activity attributes can be customized by registering an ActivityOutboundCallsInterceptor that intercepts the getLogAttributes() method.

  • Deprecated ActivityInboundLogInterceptor; registering such an interceptor with a custom logger will still work, but will prevent the use of the newly introduced getLogAttributes() interceptor. The same is true about modifying the context logger directly (eg. context.log = myCustomLogger).

  • Deprecated ActivityInboundCallsInterceptorFactory. Activity Interceptors should now be instantiated using ActivityInterceptorsFactory.

  • MockActivityEnvironment now accept a list of activity interceptors, and more closely match an actual activity execution.

Why

  • These changes adds up to better DX, notably by:
    • Promoting clear usage patterns regarding Activity logging that works out-of-the-box;
    • Discourage the use of insufficiently documented features (eg. possibility of redirecting activity logs by explicitly setting context.log = myCustomLogger, or by registering an ActivityInboundLogInterceptor with a custom logger as argument);
    • Remove the requirement that users should call appendDefaultInterceptors() if they were to register custom interceptors, which was error prone and a recurring source of support question.
  • Prepare the code base for other, similar upcoming refactors and some features (eg. custom metrics, context forwarding, etc).

@mjameswh mjameswh requested a review from a team as a code owner November 8, 2023 17:02
Co-authored-by: Spencer Judge <spencer@temporal.io>
Base automatically changed from deprecate-defaultSinks to main November 9, 2023 22:54
Comment on lines 26 to 28
static asFactory(options?: InterceptorOptions): ActivityInterceptorsFactory {
return (ctx) => ({ inbound: [new OpenTelemetryActivityInboundInterceptor(ctx, options)] });
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather not expose this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, really? TBH I feel quite the opposite...

I want to avoid requiring users to know exactly which kinds of interceptors and sinks need to be instantiated for SDK provided optional features. That evolves badly, as one day, they may need only an activity inbound interceptor, but then, at some point in the future, they would also require an activity outbound interceptor. And even though ConnectionInjectorInterceptor is not a public feature, I think applying the same pattern internally make sense.

As it currently stand, I think that gives a reasonably clean API.

const worker = await Worker.create({
  workflowsPath: require.resolve('./workflows'),
  activities,
  taskQueue: 'test-otel',
  interceptors: {
    workflowModules: [require.resolve('./workflows/otel-interceptors')],
    activity: [
      OpenTelemetryActivityInboundInterceptor.asFactory(),
      ConnectionInjectorInterceptor.asFactory(connection),
    ],
  },
  sinks,
});

Copy link
Contributor Author

@mjameswh mjameswh Nov 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now, that's essentially the same concern that led to the introduction of WorkflowCoverage.augmentWorkerOptions and friends, which is arguably an even more complete approach. The only thing I don't really like about it is the fact that it wraps around the whole WorkerOptions, which is kinda the objective of course, but has the unwanted side effect of creating some "distance" between the Worker.create and the actual options. But I can live with that.

Using that pattern instead of the asFactory() one I introduced previously, we could get something like this:

    const worker = await Worker.create(
      OpenTelemetryContextPropagator.augmentWorkerOptions(
        ConnectionInjector.augmentWorkerOptions({
          workflowsPath: require.resolve('./workflows'),
          activities,
          taskQueue: 'test-otel',
        }, connection)
      )
    );

Or, what I consider to be the best of all solutions, something similar to this:

    const worker = await Worker.create({
      workflowsPath: require.resolve('./workflows'),
      activities,
      taskQueue: 'test-otel',
      extensions: [
        OpenTelemetryContextPropagator(),
        ConnectionInjector(connection),
      ]
    })

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of that to say that I'm open to reconsider this in the interest of improving DX. In the immediate, though, I'll move on with what I already proposed in this PR. We can revisit that next week.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted asFactory for the moment. After thinking about this more, I really want to come up an approach that is not limited to activity interceptors, more in the spirit of the "extension" concept I mentioned. This will be done at a later point. In the mean time, it wouldn't be desirable to introduce a new approach (ie. asFactory) that would then be deprecated soon after.

@mjameswh mjameswh changed the title chore: Deprecate ActivityInboundLogInterceptor chore: Deprecate ActivityInboundLogInterceptor Nov 10, 2023
@mjameswh mjameswh merged commit caeb916 into main Nov 15, 2023
@mjameswh mjameswh deleted the depreacte-activity-log-interceptor branch November 15, 2023 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants