Add ClientId option on MQTT connect message #303
Conversation
This commit allows for the `client_id` to be set by the user, when performing an MQTT connect message, as specified by the MQTT protocol.
| #state_rcv{session = MqttSession}) -> | ||
| ClientId = ["tsung-", ts_utils:randombinstr(10)], |
There was a problem hiding this comment.
I'm not very deeply familiar with MQTT, but if client ID is required, we could generate it in case it is not provided.
There was a problem hiding this comment.
I had thought about that, but it seems that anyone using the protocol may quickly generate it using Tsung facilities.
Do you reckon it would be better to have another function clause which automatically generates the Client ID, when it's not provided?
There was a problem hiding this comment.
Like I said, if this is required, then I'd say yes. Otherwise this would be a breaking change.
There was a problem hiding this comment.
I've submitted a new commit which hopefully takes care of this. However, I'll need someone to validate it, since my Erlang foo is quite low.
There was a problem hiding this comment.
We also added two unit tests scenarios, providing a custom client ID or using the default implementation. So far it does not seem to be a breaking change.
|
One more thing, are you sure, you don't have to change https://github.com/processone/tsung/blob/develop/src/tsung_controller/ts_config_mqtt.erl#L47 to include the new config option? |
This commits introduces the automatic generation of the Client ID on MQTT connect, when it's missing from the MQTT Request record.
|
@tisba you're probably right, didn't notice that file. Going to take a look and provide the necessary changes. |
This commit adds the necessary config to properly use the `client_id` attribute on a MQTT connect message.
|
@tisba when you can would you mind reviewing the PR again? We'd like to start using these changes as soon as possible |
|
We've been running some local experiments and it seems the We've been looking at the code, but can't figure out what the problem is. Any help would be appreciated. EDIT: It correctly works with an hard-coded value in the EDIT2: We've fixed this in the next commit |
|
@tisba @nniclausse Could you please have at this PR, and if ok merge it ? 🙏 |
|
Hey @picaoao. This looks like a nice solution! Unfortunately I cannot merge things, only @nniclausse can :) |
|
The PR seems fine; it would be perfect if you document the new option in the mqtt part of the doc. |
|
@nniclausse I've pushed the documentation changes. Do you want me to squash all commits? |
|
that's fine. merged, thanks ! |
This PR allows for the
client_idto be set by the user, when performing an MQTT connect message, as specified by the MQTT protocol.We, at Talkdesk, need this feature as we rely on the client ID for certain validations on connect, thus we must set a custom Client ID on our Tsung clients so they may successfully connect to our MQTT brokers.
Since this ID is part of the MQTT protocol we also believe it makes sense to allow the user to set it as he wishes. If a user wants to prefix the ID with a string related to Tsung (e.g., with
tsung-as current commit does by default), he may easily do it with either a Dynamic Variable.