[Feature Request] Allow ActivityAttribute more options for name determination

[DSmith-VERX]

Is your feature request related to a problem? Please describe.

The current implementation of activity definitions makes it difficult to define reusable contracts (e.g., common classes that implement the same pattern for 1 or more activities). This is largely because of the ActivityAttribute. Without the sealed restriction, we would be able to automatically set the Name property based on our own internal logic that is based more so on the class name than the method name. In current state, every activity method implementation is required to only use the built-in naming strategy or define its own name manually to ensure they don't conflict with other classes that have the same methods.

Describe the solution you'd like

Removal of the sealed keyword on ActivityAttribute or an explanation/alternative suggestions for providing activities deterministic names that don't rely on constant/compile-time arguments.

[cretz]

explanation

We use sealed as recommended by https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1813. We did not disable this analyzer check. As mentioned there, we use things like GetCustomAttribute and performance is improved when it is sealed, though it's admittedly negligible.

alternative suggestions for providing activities deterministic names that don't rely on constant/compile-time arguments.

If you must create activities with names derived at runtime, you can use the ActivityDefinition.Create overload that accepts the name and everything. Really the methods with [Activity] are just translated to that anyways. The worker activities are just definitions despite helpers we have to extract definitions from attributes via reflection. Does this help?

[DSmith-VERX]

@cretz Thanks for the follow up. I know that's the recommendation, but it seemed like a good use case for subclassing. I don't expect you all to make any adjustments that'd measurably impact performance.

I attempted to use ActivityDefinition.Create as you suggested, but replicating some of the internal methods seemed like it wouldn't be great for maintainability considering what we're trying to avoid. I am ok with closing this for now, but please consider this for the future. There's so many more things we can do from a generics standpoint if the activity names were a little easier to override.

[cretz]

👍 I am not sure we want to support subclassing our attributes and so I think CA1813 is reasonable to leave enabled (which breaks compile unless we disable it). For our attributes, we do not recommend subclassing them.

but replicating some of the internal methods seemed like it wouldn't be great for maintainability considering what we're trying to avoid

We may be able to expose some of these helpers. So here is the logic today:

        public static ActivityDefinition Create(MethodInfo method, Func<object?[], object?> invoker)
        {
            var attr = method.GetCustomAttribute<ActivityAttribute>(false) ??
                throw new ArgumentException($"{method} missing Activity attribute");
            if (method.ContainsGenericParameters)
            {
                throw new ArgumentException($"{method} contains generic parameters");
            }
            var parms = method.GetParameters();
            return Create(
                NameFromAttributed(method, attr),
                method.ReturnType,
                parms.Select(p => p.ParameterType).ToArray(),
                parms.Count(p => !p.HasDefaultValue),
                parameters => invoker.Invoke(ParametersWithDefaults(parms, parameters)),
                method);
        }

The NameFromAttributed sounds like you may not need since you're going to be customizing them. As for ParametersWithDefaults, this is a bit of a runtime helper that only applies when using reflection-based invocation because Temporal supports extra declared parameters than what is given and falls back to defaults. We could consider exposing that if really needed though most would expect to have more exact parameters for their need and this is more of a fail-safe for Temporal since we have less control over method signatures than users might.

We would be ok exposing a helper like: public static ActivityDefinition CreateWithoutAttribute(string? name, MethodInfo method, Func<object?[], object?> invoker) that does basically everything the Create does above except it doesn't try to get the attribute (we have done similarly with WorkflowSignalDefintion.CreateWithoutAttribute).

Also what some people do is use a dynamic activity. Each worker can have one [Activity(Dynamic = true)] activity that accepts a IRawValue[] args parameter. Then it is called for all activity invocations where there isn't an explicitly registered one. You can use ActivityExecutionContext.Current.Info.ActivityType to get the name and do whatever you want (ActivityExecutionContext.Current.DataConverter can be used to convert IRawValue to what you want).

[DSmith-VERX]

We would be ok exposing a helper like: public static ActivityDefinition CreateWithoutAttribute(string? name, MethodInfo method, Func<object?[], object?> invoker) that does basically everything the Create does above except it doesn't try to get the attribute (we have done similarly with WorkflowSignalDefintion.CreateWithoutAttribute).

I'm likely going down the dynamic route for today, but this would be perfect, preferred, and very much appreciated.

[cretz]

:+1: I have changed the title to reflect that ActivityDefinition.CreateWithoutAttribute helper would help. We can have this accept attribute options without the attribute itself, similar to how Create works without attribute today, but keeping some of those other values.