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

Nueva clase IoT #70

Merged
merged 8 commits into from
Nov 28, 2023
Merged

Nueva clase IoT #70

merged 8 commits into from
Nov 28, 2023

Conversation

CeciliaFili
Copy link

@CeciliaFili CeciliaFili commented Nov 21, 2023

Edit (fgalan): issue #55

  • Se creó la clase IoT capaz de enviar datos al agente IoT. Tiene dos métodos:

    1. send_json: envía datos en formato JSON.
    2. send_batch: envía un conjunto de diccionarios. Recibe un tiempo de espera entre cada envío para evitar que el agente se sature. Es capaz de recibir un DataFrame. La función se encarga de convertir cada fila en un diccionario y luego los envía uno por uno.

Se pueden encontrar más detalles de las funciones en los archivos best_practices.md y README.md

  • Se movió la implementacion de FetchError a un archivo independiente exceptions.py para centralizar el manejo de excepciones generales.

  • Para asegurar la robustez del código se añadieron pruebas unitarias.

  • Se agregó la librería pandas para el manejo de DataFrames. Debe ejecutarse pip install -r requirements.txt para actualizar las dependencias.

@fgalan
Copy link
Contributor

fgalan commented Nov 22, 2023

Tras revisarlo en conjunto con @arcosa (autor principal de esta libreria) os pasamos el siguiente feedback:

Como comentario principal a la PR, sería conveniente homogeneizar a como se hace con otras clases y adoptar un patrón de controladores para los componentes de plataforma. Es decir, al igual que tenemos un cbManager para el componente Context Broker, que se instancia y luego se usan métodos con él (pe. send_batch, get_entities, etc.), sería conveniente hacer algo parecido para los IOTAs. En concreto, definir una iotaManager cuyo constructor use estos parámetros:

  • req_url
  • api_key

y los métodos send_json y send_batch ya no tendrían req_url y api_key.

Con respecto a time_sleep, también por homogeneidad haría como en el caso del CB: renombrarlo a sleep_send_batch con un valor por defecto (en el cb es 0, es decir no se espera entre envíos) y definido en el constructor para no repetirlo en las llamadas send_json y send_batch. Por otro lado, en entornos productivos es posible que se produzcan microcortes de conexión cuando se quieren enviar datos, sería aconsejable incorporar un mecanismo timeout/try similar al que se usa en el caso del CB, que tenemos un timeout (10 por defecto), post_retry_connect (3 por defecto), post_retry_backoff_factor (20 por defecto) definidos en el manager, que se pueden modificar a través del constructor y que send_batch (en este caso send_json) le da uso, en caso de tener timeouts hacer un número de retrys automáticamente, dará estabilidad en el envío de esos datos. Este mecanismo de timeout/try igual se puede dejar en deuda (en tal caso, no estaría dentro del scope de esta PR y abriríamos un issue sobre ello, decidnos por favor vuestro parecer).

El soporte de dataframes en el método de send_batch nos parece una buena idea a fin de aumentar la flexibilidad de uso de la librería. Hemos abierto un issue (#71) para incorporar esta misma funcionalidad en el send_batch del CB (quedando fuera del scope de esta PR). En todo caso, en vez de tocar en etls/hello-world/requirements.txt, si se quiere indicar que pandas es un requisito de la librería, sería en el INSTALL_REQUIRES de python-lib/tc_etl_lib/setup.py

(A continuación habrá una sugerencia de renaming de algunas cosas, pero hasta aquí me refiero a los nombres antiguos, para no liar).

Luego, algunas consideraciones adicionales:

  • Algunos renombrados:
    • iot.py -> iota.py (y test_iot.py -> test_iota.py).
    • req_url -> endpoint (por homogeneidad por parámetros similares existentes)
    • sensor_name / nombre de sensor -> device_id / ID de dispositivo (que creo que el término que usa la API de los IOTAs para referirse a este concepto)
    • _json en send_json puede ser un poco redundante (aunque tenemos IOTAs que no son de JSON, pe. IOTA-UL, no están ahora mismo en el foto de atención) pero si se quiere dar una notición del tipo de formato en el nombre del método (cosa que no me parece mal) mejor referirnos al transporte más que al formato. En este sentido, podríamos usar send_http y send_batch_http (por si en el futuro queremos extender con pe. un send_mqtt y send_batch_mqtt).
  • En la documentación, usar links a los tipos de excepción, como se hace ya en algún otro caso existente, pe. :raises [ValueError]([https://docs.python.org/3/library/exceptions.html#ValueError)](https://docs.python.org/3/library/exceptions.html#ValueError)%60)
  • Estaría bien incluir algún ejemplo de uso de IOTAs en https://github.com/telefonicasc/etl-framework/tree/master/python-lib/tc_etl_lib#uso-de-la-librer%C3%ADa, estilo a los que ya hay, que son estilo autocontenidos

@xavi12p
Copy link

xavi12p commented Nov 23, 2023

Hola, respecto a esto:

Por otro lado, en entornos productivos es posible que se produzcan microcortes de conexión cuando se quieren enviar datos, sería aconsejable incorporar un mecanismo timeout/try similar al que se usa en el caso del CB, que tenemos un timeout (10 por defecto), post_retry_connect (3 por defecto), post_retry_backoff_factor (20 por defecto) definidos en el manager, que se pueden modificar a través del constructor y que send_batch (en este caso send_json) le da uso, en caso de tener timeouts hacer un número de retrys automáticamente, dará estabilidad en el envío de esos datos. Este mecanismo de timeout/try igual se puede dejar en deuda (en tal caso, no estaría dentro del scope de esta PR y abriríamos un issue sobre ello, decidnos por favor vuestro parecer).

Estamos de acuerdo en incorporar ese mecanismo de timeout/try y vemos adecuado que se aborde posteriormente, como deuda técnica.

Corregimos el resto de temas que nos indicáis y actualizamos el PR.

@fgalan
Copy link
Contributor

fgalan commented Nov 23, 2023

Hola, respecto a esto:

Por otro lado, en entornos productivos es posible que se produzcan microcortes de conexión cuando se quieren enviar datos, sería aconsejable incorporar un mecanismo timeout/try similar al que se usa en el caso del CB, que tenemos un timeout (10 por defecto), post_retry_connect (3 por defecto), post_retry_backoff_factor (20 por defecto) definidos en el manager, que se pueden modificar a través del constructor y que send_batch (en este caso send_json) le da uso, en caso de tener timeouts hacer un número de retrys automáticamente, dará estabilidad en el envío de esos datos. Este mecanismo de timeout/try igual se puede dejar en deuda (en tal caso, no estaría dentro del scope de esta PR y abriríamos un issue sobre ello, decidnos por favor vuestro parecer).

Estamos de acuerdo en incorporar ese mecanismo de timeout/try y vemos adecuado que se aborde posteriormente, como deuda técnica.

Perfecto. Creado issue al respecto #72 y queda fuera del scope de la PR #70

@@ -112,6 +112,10 @@ entities = cbm.get_entities_page(subservice='/energia', auth=auth, type='myType'

# have a look to the retrieved entities
print (json.dumps(entities))

Copy link
Contributor

Choose a reason for hiding this comment

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

Creo que sería mejor dejar los ejemplos que ya hay como están y usar los dos nuevos fragmentos para generar un ejemplo separado independiente.

- :param obligatorio `sensor_id`: El ID del sensor.
- :param obligatorio `api_key`: La API key correspondiente al sensor.
- :param obligatorio `endpoint`: La URL del servicio al que se le quiere enviar los datos.
- :param opcional `sleep_send_batch`: Es el tiempo de espera entre cada envío de datos en segundos.
Copy link
Contributor

Choose a reason for hiding this comment

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

Siendo un parámetro opcional, indicar el default cuando no se especifica nada.

- :param obligatorio: `data`: Datos a enviar. Puede ser una lista de diccionarios o un DataFrame.
- :raises SendBatchError: Se levanta cuando se produce una excepción dentro de `send_http`. Atrapa la excepción original y se guarda y se imprime el índice donde se produjo el error.


Copy link
Contributor

Choose a reason for hiding this comment

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

Incluir en el changelog (justo por encima de la marca de versión 0.9.0) un par de entradas que describan los cambios incluidos en esta PR. Por ejemplo:

  • Add: new class iotaManager to deal with IOTAgent interactions, with methods send_http and send_batch_http

Comment on lines 508 to 509
0.10.0 (November 27th, 2023)

Copy link
Contributor

Choose a reason for hiding this comment

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

Los cierres de versión los hacemos normalmente independientemente a la funcionalidad (por si entra alguna cosa más antes de cerrar versión... en este caso tal vez no, pero por procedimiento lo hacemos así).

Suggested change
0.10.0 (November 27th, 2023)

Por tanto, por favor eliminar estas 2 lineas (como marca el suggestion) en esta PR. Con aceptar el suggestion valdría.

Copy link
Collaborator

@arcosa arcosa left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@fgalan fgalan left a comment

Choose a reason for hiding this comment

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

LGTM

¡Gracias por la excelente contribución! :)

@fgalan fgalan merged commit 09db6c1 into telefonicasc:master Nov 28, 2023
1 check passed
@mapedraza
Copy link
Collaborator

Revisando la clase iotaManager, y sus métodos asociados, no es posible hacer un envío múltiple para distintos IDs. Esto sería útil para no tener que construir un objeto para cada uno de los IDs a enviar (si hay que actualizar múltiples dispositivos al final de una ETL).

Sería interesante la clase dispusiera de un método (i.e: send_http_multienty) en el cual se le pudiera pasar un diccionario con los ids de dispositivos como claves y el paquete de envío como valor.

En cualquier caso el apikey podría ser estático (normalmente no va a cambiar el apikey por ETL)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants