Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

swift: add core #285

Merged
merged 11 commits into from
Feb 11, 2025
Merged

swift: add core #285

merged 11 commits into from
Feb 11, 2025

Conversation

fsuc
Copy link
Member

@fsuc fsuc commented Feb 5, 2025

What's new

Add a implementation of the Core which uses a MQTTClient and a TelemetryClient to perform MQTT operations with traces.


How to test

  1. Clone the its-client project and check out the branch under review
  2. Run a local OpenTelemetry collector with a http endpoint on port 4318:
     $ docker container run \
          --rm \
          -p 16686:16686 \
          -p 4318:4318 \
          jaegertracing/all-in-one:1.58
  3. Run the test CoreTests.message_sent_to_subscribed_topic_should_be_received_and_create_linked_span:
    swift test --filter CoreTests.message_sent_to_subscribed_topic_should_be_received_and_create_linked_span   
    Expected result: Check on the Jaeger UI (on http://localhost:16686/) that that 1 producer span and 1 consumer span which references the producer have been created.
  4. Run the test CoreTests.message_sent_with_error_should_create_span_with_error:
    swift test --filter CoreTests.message_sent_with_error_should_create_span_with_error   
    Expected result: Check on the Jaeger UI (on http://localhost:16686/) that 1 producer span has been created with an error.
  5. Run the test CoreTests.send_message_without_telemetry_should_not_create_span:
    swift test --filter CoreTests.send_message_without_telemetry_should_not_create_span   
    Expected result: Check on the Jaeger UI (on http://localhost:16686/) that no span have been created.

Closes #284

fsuc added 7 commits January 31, 2025 17:25
Signed-off-by: François Suc <francois.suc@orange.com>
Remove the closure from the initializer and add a property to set it later

Signed-off-by: François Suc <francois.suc@orange.com>
Signed-off-by: François Suc <francois.suc@orange.com>
Core uses a MQTTClient and an optional TelemetryClient to receive and send messages from MQTT broker with telemetry if opt-in

Signed-off-by: François Suc <francois.suc@orange.com>
Signed-off-by: François Suc <francois.suc@orange.com>
Signed-off-by: François Suc <francois.suc@orange.com>
Signed-off-by: François Suc <francois.suc@orange.com>
@fsuc fsuc added the Swift label Feb 5, 2025
@fsuc fsuc self-assigned this Feb 5, 2025
@fsuc fsuc requested a review from ymorin-orange February 5, 2025 17:02
func unsubscribe(from topic: String) async throws(MQTTClientError) {
guard isConnected else {
throw .clientNotConnected
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, this guard ensures that we can only unsubscribe (but there is the same guard on the subscribe) when the MQTT client is actually connected.

This has too implications, both not very pleasant:

  1. the user of the SDK may not be aware that the MQTT client got disconnected, so they will get an exception; and even if they checked whether the MQTT client was connected before unsubscribing, this is both tedious and not race-free;
  2. the MQTT client does not automatically re-subscribe on its own, and the user of the SDK has to track subscriptions on their own; since they have no way of knowing if/when the MQTT client gets dis/connected, they have no way to know if/when they have to re-subscribe to their topics of interest.

What would be a better solution is to actually keep the list of subscriptions as a "private member" in the MQTT client object, and allow the user of the SDK to manipulate that list at will (by way of calling subscribe() and unsibscribe()). Then when the MQTT client gets connected, it requests those subscriptions to the broker.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the first point the library does the same : it launches an exception if not connected so it would be the same result without the guard for all the operations.
I plan to manage the disconnection later because there are two main options:

  1. just trigger a disconnection event for the client. The client must stop publishing messages or update subscriptions and handle a new connection/subscriptions depending its network status (network is on -> reconnect) or a retry mechanism if the disconnection is not related to the mobile network.
  2. Handle this internally with a reconnection mechanism that can connect and resume the subscriptions. In this case, we need to choose the behavior when a message is sent when reconnecting (skip, save to send later ?). This solution is easy to use but opaque and can be related to the use case of the client regarding the behaviors when reconnecting.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Option retained : 2 for a future PR.
Update subscriptions list and skip published message when reconnecting.

fsuc added 4 commits February 7, 2025 17:37
Used to build a new message to publish

Signed-off-by: François Suc <francois.suc@orange.com>
Signed-off-by: François Suc <francois.suc@orange.com>
Signed-off-by: François Suc <francois.suc@orange.com>
It allows to enhance span status description when an error occurs.

Signed-off-by: François Suc <francois.suc@orange.com>
@ymorin-orange ymorin-orange self-requested a review February 11, 2025 05:24
Copy link
Member

@ymorin-orange ymorin-orange left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Code peer-reviewed
    • Comments to be addressed as evolutions, so in future PRs.
  • Tests OK

@fsuc fsuc merged commit b0f58b8 into Orange-OpenSource:master Feb 11, 2025
58 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Core
2 participants