Added option to turn shared subscription off or change the group identifier

[fbuedding]

I've noticed that the IoT-Agent UL is using shared MQTT subscriptions. On top of that I couldn't find any documentation regarding to this. It was introduced in this commit.

While I think the reasoning behind this is correct and needed, I don't think there is any drawback from adding a feature to make this more configurable.

Most of the changed lines are coming from the configured pre-commit linting hook.

Use cases:

• In case multiple FIWARE Platforms with the same IoT-Agent are connected to one MQTT Broker messages are not send to every IoT-Agent
• When testing it is a nice to have the option to turn shared subscriptions off
• This pull request also documents that the IoT-Agent is using shared subscriptions
• Defining the group identifier could help when multiple Context Broker and IoT-Agents are used for load balancing

Behaviour

• If the 'IOTA_MQTT_DISABLE_SHARED_SUBSCRIPTIONS' environment variable is set to 'true', shared subscriptions are turned off. In every other case it stays on
• If the 'IOTA_MQTT_GROUP_ID_SUFIX' environment variable is set, the value is appended to the group identifier
• ==> Backwards compatible

Changes made:

• added 'IOTA_MQTT_DISABLE_SHARED_SUBSCRIPTIONS' and 'IOTA_MQTT_GROUP_ID_SUFIX' environment variables
• added tests
• Documentation
• linting (this changed quite some formatting but is configured as pre-commmit hook)

All in all this is a minor change since it is backwards compatible and only adds features for edge cases. All tests are passing (The three pending tests were also pending before and are located in 'HTTP Transport binding: polling commands').

145 passing (14s)
3 pending

File	% Stmts	% Branch	% Funcs	% Lines	Uncovered Line #s
All files	86.94	73.86	92.94	86.84
lib	88.69	83.63	92.22	88.57
commonBindings.js	96.93	85.71	100	96.9	293,305-306
configService.js	93.54	91.91	100	93.49	49-50,150,153,156,224,228,232
constants.js	100	100	100	100
errors.js	34.78	100	28.57	34.78	33-66
iotaUtils.js	89.07	82.82	100	88.88	47-48,59-60,91,106-117,145,182,202,206-215
iotagent-ul.js	82.53	56.52	88.23	82.53	78-86,107,123,179,226-232
transportSelector.js	100	100	100	100
ulParser.js	87.82	79.71	100	87.61	45,62,132,225,247-262
lib/bindings	84.82	59.27	93.75	84.76
AMQPBinding.js	79.13	43.47	95	79.13	124,127,133,139,147,154,159,167,170-172,184,191-194,198-199,205-208,225,263
HTTPBindings.js	83.51	71.42	97.22	83.42	...,115,119,124,128,132,171,195,201-202,229-231,251,308,320-327,475-480,497
MQTTBinding.js	90.3	54.76	87.5	90.24	261,265,291,305,342,346,378,387-390,395-397,407,410,436

[mapedraza]

@fbuedding Thank you so much for your contribution. This PR LGTM. Before merging is required a twin PR under https://github.com/telefonicaid/iotagent-json repo adding the same feature.

Once we have open that PR, we can merge both

[fbuedding]

@fbuedding Thank you so much for your contribution. This PR LGTM. Before merging is required a twin PR under https://github.com/telefonicaid/iotagent-json repo adding the same feature.

Once we have open that PR, we can merge both

Hello! Thank you!

Do I have to open the twin PR?

Sorry in advanced if I missed that!

[fgalan]

@fbuedding Thank you so much for your contribution. This PR LGTM. Before merging is required a twin PR under https://github.com/telefonicaid/iotagent-json repo adding the same feature.
Once we have open that PR, we can merge both

Hello! Thank you!

Do I have to open the twin PR?

Yes. That's correct. Please open a twin PR in the https://github.com/telefonicaid/iotagent-json repo, with the same feature implemented here.

Sorry in advanced if I missed that!

Don't worry! :)

[AlvaroVega]

In order to not duplicate all topics code:

Why not
if (config.getConfig().mqtt.sharedSubscriptionsDisabled === true)
MQTT_SHARE_SUBSCRIPTON_GROUP_VAR = "";
else
MQTT_SHARE_SUBSCRIPTON_GROUP_VAR = constants.MQTT_SHARE_SUBSCRIPTION_GROUP

and then use the same block but instead of constants.MQTT_SHARE_SUBSCRIPTION_GROUP use MQTT_SHARE_SUBSCRIPTON_GROUP_VAR ?

[fbuedding]

My original thinking was that I wanted to keep as much original code as possible, but the way you described is indeed better and more elegant. Changed it and tests are passing.

[fbuedding]

I'm not sure why the tests are failing or timing out, here are my results for Node 14.x

145 passing (14s)
3 pending

File	% Stmts	% Branch	% Funcs	% Lines	Uncovered Line #s
All files	86.45	73.68	92.94	86.35
lib	88.69	83.63	92.22	88.57
commonBindings.js	96.93	85.71	100	96.9	293,305-306
configService.js	93.54	91.91	100	93.49	49-50,150,153,156,224,228,232
constants.js	100	100	100	100
errors.js	34.78	100	28.57	34.78	33-66
iotaUtils.js	89.07	82.82	100	88.88	47-48,59-60,91,106-117,145,182,202,206-215
iotagent-ul.js	82.53	56.52	88.23	82.53	78-86,107,123,179,226-232
transportSelector.js	100	100	100	100
ulParser.js	87.82	79.71	100	87.61	45,62,132,225,247-262
lib/bindings	83.66	58.82	93.75	83.59
AMQPBinding.js	79.13	43.47	95	79.13	124,127,133,139,147,154,159,167,170-172,184,191-194,198-199,205-208,225,263
HTTPBindings.js	83.51	71.42	97.22	83.42	51-59,83,87,115,119,124,128,132,171,195,201-202,229-231,251,308,320-327,475-480,497
MQTTBinding.js	87.33	53.57	87.5	87.24	143-145,243,247,273,287,324,328,360,369-372,377-379,389,392,418

[mapedraza]

You should remove this file from PR

[fbuedding]

I investigated why the test are sometimes failing and I think it is just some timeout issue.

After that I tried to reproduce the same on the unmodified branch and the tests are also sometimes failing. After my changes it became more frequent since I'm repeating the tests with different configurations.

If there is something I can do or I have to do, pls let me know @AlvaroVega @mapedraza

Kind regards

[fgalan]

I investigated why the test are sometimes failing and I think it is just some timeout issue.

After that I tried to reproduce the same on the unmodified branch and the tests are also sometimes failing. After my changes it became more frequent since I'm repeating the tests with different configurations.

If there is something I can do or I have to do, pls let me know @AlvaroVega @mapedraza

Kind regards

You are right. There is 1 test case related with MQTT that sometime fails. A re-run of the tests use to fix the problem, although it is a annoying glitch.

If the changes introduced by your PR increase the probablity of this fail, the severity of this problem could change from "annyoing" to "blocking". Thus, maybe it's a good moment to investigate the cause of the problem and provide a fix. That could be done in an independent PR, for the sake of clearness.

@fbuedding what do you think? could you do that investigation/fix proposal, please?

[fbuedding]

I investigated why the test are sometimes failing and I think it is just some timeout issue.
After that I tried to reproduce the same on the unmodified branch and the tests are also sometimes failing. After my changes it became more frequent since I'm repeating the tests with different configurations.
If there is something I can do or I have to do, pls let me know @AlvaroVega @mapedraza
Kind regards

You are right. There is 1 test case related with MQTT that sometime fails. A re-run of the tests use to fix the problem, although it is a annoying glitch.

If the changes introduced by your PR increase the probablity of this fail, the severity of this problem could change from "annyoing" to "blocking". Thus, maybe it's a good moment to investigate the cause of the problem and provide a fix. That could be done in an independent PR, for the sake of clearness.

@fbuedding what do you think? could you do that investigation/fix proposal, please?

@fgalan Yes I will look into this!

[fgalan]

@fbuedding it seems this PR has been closed... could you elaborate on the reason, please? Thanks!

[fbuedding]

@fbuedding it seems this PR has been closed... could you elaborate on the reason, please? Thanks!

Hello,
I closed it, because something is wrong with my fork. The build docker image always errored and exited. I then verified, that it has to do something with what I did. Syncing my fork also made matters worse, thus i thought a fresh and clean pull request would be better.

I'm sorry if thats the wrong pull request etiquette, this is the first one I ever did on a bigger project.

[fgalan]

No problem. It's perfectly fine with request etiquette :) I was just to know the reason behind the closing.