Artemis Scaler parses out broker config parameters in case restAPITemplate is given

[Ritikaa96]

Adding functionality to provide brokerConfig parameters in ActiveMQ Artemis scaler from restAPITemplate.

In reference to #2097

Checklist

• Commits are signed with Developer Certificate of Origin (DCO - learn more)
• Tests have been added
• Changelog has been updated

Fixes #2097

[Ritikaa96]

@ahmelsayed @zroubalik PTAL

[Ritikaa96]

Hi @zroubalik , @tomkerkhove , @balchua I've added the functionality to parse out the parameters from restApiTemplate. PTAL

Now there are two options.

• we add a check to throw an error to users not to provide both restApiTemplate & brokerConfig in metadata.
• we just mention this information in our scaler's document because when we provide restAPITemplate the code doesn't try to search values of the parameters from config.

what do you suggest?

[balchua]

I was wondering if there is a need to verify the restApiTemplate here. I don't know how much maintenance this will incur and keeping it up to date.
Since a wrong path and request parameters will yield some http errors.

[Ritikaa96]

I was wondering if there is a need to verify the restApiTemplate here. I don't know how much maintenance this will incur and keeping it up to date.
Since a wrong path and request parameters will yield some http errors.

Hi @balchua I've added verification if the parameters are given or not for the parser. If the parsing path is wrong and not valid generally the code will throw Error .Can you share any Example case for the wrong restAPITemplate so we can make sure it doesn't raise concern later on.

[balchua]

Hi @Ritikaa96 just an invalid endpoint will do.

I also recall someone tried to hook this up to ActiveMQ classic, which worked since the jmx output is similar to artemis. But the rest API template is different. The discussion was posted in slack.
Is this something that keda would like to have?
Generally I don't have objections on validating the url like what you did. I was just wondering if this will be too much of a maintenance burden.

[Ritikaa96]

Hi @balchua @tomkerkhove @zroubalik , as we discussed in #2097 , the above change goes with allowing only one way at a time to provide the borkerConfig. If the use causes more maintenance issues then maybe we can go for second option and remove restApiTemplate from metadata.
Furthermore, restApiTemplate is generated using brokerConfig no matter if we have provided it in metadata acc to scaler code here
this function's references are getMetrics & isActive from artemis code already.

Please let me know what you think about removing restApiTemplate altogether as its already been generated from the default template.

[Ritikaa96]

Hi @Ritikaa96 just an invalid endpoint will do.

I also recall someone tried to hook this up to ActiveMQ classic, which worked since the jmx output is similar to artemis. But the rest API template is different. The discussion was posted in slack. Is this something that keda would like to have? Generally I don't have objections on validating the url like what you did. I was just wondering if this will be too much of a maintenance burden.

@balchua
I just checked the code for ActiveMQ Classic and the parser is just the same there. The maintenance concern might stay for the ActiveMQ classic code too. Am I missing something here? Please let me know.

[balchua]

I didn't look at the details of activemq classic carefully tho.

I am not sure about the deprecation policy of KEDA, coz removing the restApiTemplate will break existing installations when they upgrade.

A PR to include activemq classic. 😃
#2121

[Ritikaa96]

I didn't look at the details of activemq classic carefully tho.

I am not sure about the deprecation policy of KEDA, coz removing the restApiTemplate will break existing installations when they upgrade.

A PR to include activemq classic. smiley #2121

@balchua yes i saw the code and changes. mostly are same, at least the restApiTemplate parser is so the real question is that if this change is acceptable or the maintainers wants to have some other changes because either way the code having both restapitemplate and broker config from metadata at the same time seems redundant as it is regenerated through the brokerConfig params while getting the metrics and also checking for the active state.

[balchua]

I digged out why the rest endpoint was added.

#986
It looks like the endpoint can have different contents.

[Ritikaa96]

@balchua I see. I might be wrong but it seems like we are still using the same default template to access the endpoint either way. as per this code here .
the queueMessageCount function is called upon by GetMetrics and IsActive functions. this function here call to getMonitoringPoint everytime without an exception which generate the endpoint in our default template form. So either way we are using the same template for endpoint.

func (s *artemisScaler) getQueueMessageCount() (int, error) {
	var monitoringInfo *artemisMonitoring
	messageCount := 0

	client := s.httpClient
	url := s.getMonitoringEndpoint()
......

func (s *artemisScaler) IsActive(ctx context.Context) (bool, error) {
	messages, err := s.getQueueMessageCount()
........

func (s *artemisScaler) GetMetrics(ctx context.Context, metricName string, metricSelector labels.Selector) ([]external_metrics.ExternalMetricValue, error) {
	messages, err := s.getQueueMessageCount()

	if err != nil {
		artemisLog.Error(err, "Unable to access the artemis management endpoint", "managementEndpoint", s.metadata.managementEndpoint)
		return []external_metrics.ExternalMetricValue{}, err
	}
.......

func (s *artemisScaler) getMonitoringEndpoint() string {
	replacer := strings.NewReplacer("<<managementEndpoint>>", s.metadata.managementEndpoint,
		"<<queueName>>", s.metadata.queueName,
		"<<brokerName>>", s.metadata.brokerName,
		"<<brokerAddress>>", s.metadata.brokerAddress)

	monitoringEndpoint := replacer.Replace(s.metadata.restAPITemplate)

	return monitoringEndpoint
}

[Ritikaa96]

@balchua If the monitoring is done by the default restApi content anyways then it is possible the current users are only utilising the same for now which makes it redundant to regenerate the same API that's given by metadata.
To include possibly different restAPI , we can do something like this perhaps:

func (s *artemisScaler) getQueueMessageCount() (int, error) {
	var monitoringInfo *artemisMonitoring
	messageCount := 0

	client := s.httpClient
        if s.metadata.restApiTemplate != " " {
         url:= s.metadata.restApiTemplate
         }
	else {
          url := s.getMonitoringEndpoint()
        }

	req, err := http.NewRequest("GET", url, nil)

	req.SetBasicAuth(s.metadata.username, s.metadata.password)
......

What do you think? it will include every restAPITemplate's value, and our code won't regenerate the url without needing it.

[balchua]

@Ritikaa96 i am ok with your proposal. So in short, if a user uses the restApiTemplate (its no longer a template in this case) it is up to the user to verify the validity of the endpoint provided.
I think this is fine to me.

[Ritikaa96]

@balchua If the monitoring is done by the default restApi content anyways then it is possible the current users are only utilising the same for now which makes it redundant to regenerate the same API that's given by metadata. To include possibly different restAPI , we can do something like this perhaps:

func (s *artemisScaler) getQueueMessageCount() (int, error) {
	var monitoringInfo *artemisMonitoring
	messageCount := 0

	client := s.httpClient
        if s.metadata.restApiTemplate != " " {
         url:= s.metadata.restApiTemplate
         }
	else {
          url := s.getMonitoringEndpoint()
        }

	req, err := http.NewRequest("GET", url, nil)

	req.SetBasicAuth(s.metadata.username, s.metadata.password)
......

What do you think? it will include every restAPITemplate's value, and our code won't regenerate the url without needing it.

@balchua thank you , so should i update the current code to this above ?

[balchua]

Yes for me. 😃

[zroubalik]

Looking good, could you please note this change in Changelog?

[Ritikaa96]

Looking good, could you please note this change in Changelog?

Hi @zroubalik , what do you think about the discussion above? I just tried, as per the discussion with @balchua , to make the repetitive regeneration of restAPiTemplate conditional but then if restAPITemplate is not valid it would throw an error for sure and it will depend on the user to confirm their input was correct.
Please let me know after you go through the discussion.

[Ritikaa96]

@ahmelsayed @zroubalik PTAL. I've added the change in CHANGELOG. I'd also like to know what is your opinion about the discussion above. My first preference would be having broker config parameters when restAPITemplate is not provided as to when we regenerate the monitoringEndpoints , we use our default endpoint as a reference which guarantees that the format of restAPITemplate in use is the same in all cases. So in this case, if restAPITemplate has a different content, even then GetMetrics and isActive regenerate the default restAPITemplate as per the code leaving the original API as redundant information.

[Ritikaa96]

@ahmelsayed @zroubalik
Please let me know if any other changes are to be done

[zroubalik]

LGTM