[Alerting] Improved type check for action variables

[sorenlouv]

Type: enhancement
Priority: Low

---

Currently it's easy to mess up the action variables by forgetting to either

1. declare action variables in actionVariables.context (1)
2. specify the value for declared action variables alertInstance.scheduleActions (2)

✅ Correct

The following declares action variables and specifies their values correctly:

export const apmActionVariables = {
  serviceName: {
    description: "The service the alert is created for",
    name: "serviceName",
  },
  transactionType: {
    description: "The transaction type the alert is created for",
    name: "transactionType",
  },
  environment: {
    description: "The transaction type the alert is created for",
    name: "environment",
  },
};

alerts.registerType({
  actionVariables: {
    context: [
      // Declare action variables (1)
      apmActionVariables.serviceName,
      apmActionVariables.transactionType,
    ],
  },
  // ...
  executor: async ({ services }) => {
    const alertInstance = services.alertInstanceFactory("alertInstanceName");
    alertInstance.scheduleActions("defaultActionGroupId", {
      // Set action variable values (2)
      serviceName: 'my service name',
      transactionType: 'my transaction type'
    });
  },
});

❌ Incorrect

In the following the action variable declarations don't match the values specified in scheduleActions:

alerts.registerType({
  actionVariables: {
    context: [
      // Declare action variables (1)
      apmActionVariables.serviceName,
      apmActionVariables.environment,
    ],
  },
  // ...
  executor: async ({ services }) => {
    const alertInstance = services.alertInstanceFactory("alertInstanceName");
    alertInstance.scheduleActions("defaultActionGroupId", {
      // Set action variable values (2)
      serviceName: 'my service name',
      transactionType: 'my transaction type' // this is not declared above. Should not be allowed. 
      // environment is declared above but the value it missing here
    });
  },
});

Question

Would it be possible to narrow the types for alertInstance.scheduleActions so that action variables declared in (1) will be required?

[elasticmachine]

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

[gmmorris]

specify the value for declared action variables alertInstance.scheduleActions

I'm already working on this as part of: #83501
It still isn't clear how easy it will be to enforce the typing properly due to several layers of indirection (Recovery groups are dynamic evaluated at registration which makes this even harder), but I'm hoping to figure it out. :)

[gmmorris]

@sqren - mind checking if the above issue addressed this to your satisfaction? :)
If there are still edge cases that weren't covered by #83501 we can take another stab at it.

[sorenlouv]

Hey @gmmorris

What I was hoping was that based on actionVariables.context:

kibana/x-pack/plugins/apm/server/lib/alerts/register_transaction_duration_alert_type.ts

Lines 70 to 79 in 17443d0

	actionVariables: {
	context: [
	apmActionVariables.serviceName,
	apmActionVariables.transactionType,
	apmActionVariables.environment,
	apmActionVariables.threshold,
	apmActionVariables.triggerValue,
	apmActionVariables.interval,
	],
	},

I'd get suggestions for the context in scheduleActions:

kibana/x-pack/plugins/apm/server/lib/alerts/register_transaction_duration_alert_type.ts

Lines 170 to 177 in 17443d0

	.scheduleActions(alertTypeConfig.defaultActionGroupId, {
	transactionType: alertParams.transactionType,
	serviceName: alertParams.serviceName,
	environment: getEnvironmentLabel(alertParams.environment),
	threshold,
	triggerValue: transactionDurationFormatted,
	interval: `${alertParams.windowSize}${alertParams.windowUnit}`,
	});

But even if I delete everything in scheduleActions I don't get any compile errors:

[gmmorris]

@sqren I think this is because you're using the Rule Registry and it seems to be overwriting the generics.
At framework level we let you specify the type of the context, but from a quick look, I think the RuleRegistry has hard coded all Context types as Record<string, unknown>.

kibana/x-pack/plugins/rule_registry/server/utils/create_lifecycle_rule_type_factory.ts

Line 35 in 17443d0

export type LifecycleAlertService<TAlertInstanceContext extends Record<string, unknown>> = (alert: {

From a framework perspective, what you're asking for should already work.
I suggest chatting with @dgieselaar about it (though I know he isn't working on the RuleRegistry anymore but he'll likely know the context of this decision)

[sorenlouv]

Okay, that makes sense. Thanks for the explanation. Closing.