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

Create SlicingDice sink #1542

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

joaosimbiose
Copy link

@joaosimbiose joaosimbiose commented Nov 24, 2018

This PR resolves the issue #1541

The SlicingDice's development team wrote this Cygnus extension for a new sink to persist data in SlicingDice's databases.

Please, we kindly ask you to review this pull request and provide us your insights and considerations about the implementation.

We read and followed all the contribution guidelines [1], documenting the code and also creating all the necessa1y tests.

[1] - https://github.com/telefonicaid/fiware-cygnus/blob/master/doc/contributing/contributing_guidelines.md#contributing-guidelines

Thanks in advance.


SlicingDice API Overview

SlicingDice provides a REST API [2] that allows its customers to insert (and query) data using the API.

In order to insert values into a SlicingDice database, you send a POST request to https://api.slicingdice.com/v1/insert with a JSON object which the key is the entity ID containing JSON objects with database's column names and their respective values.

In the Cygnus case, our implementation considers the entity ID to be the IoT device generating the data.

Any timeseries-related data (metrics) sent by Cygnus will be stored in a SlicingDice database on an Event Column and the non-timeseries data on an Attribute Column.

An example of the API insertion request generated by this Cygnus extension would look like this:

curl -X POST https://api.slicingdice.com/v1/insert \
    -H 'Authorization: DATABASE_API_KEY' \
    -H 'Content-Type: application/json' \
    -d '{
        "DeviceID": {
            "dimension": "default",
            "sensor-type": "Types of the Sensor",
            "sensor-model": "Model XYZ",
            "sensor-firmware-version": "1.3.2",
            "temperature": [
                {
                    "value": 78,
                    "date": "2018-11-14T12:54:12Z"
                }
            ],
            "pression": [
                {
                    "value": 13,
                    "date": "2018-11-14T12:54:12Z"
                }
            ],
            "moisture": [
                {
                    "value": 20,
                    "date": "2018-11-14T12:54:12Z"
                }
            ],
            "sensor-status": [
                {
                    "value": "active",
                    "date": "2018-11-14T12:54:12Z"
                }
            ]
        },
        "auto-create": ["dimension", "column"]
    }'

Implementation considerations:

  • SlicingDice's API supports HTTP and HTTPS.
  • SlicingDice's Cygnus extension implementation is by default leveraging the SlicingDice's ability to create new columns and dimension as needed, if they don't exist on the database, by adding the parameter "auto-create": ["dimension", "column"] at the end of all insertion requests. This allows this integration to never break in case Cygnus adds new columns.

[2] - SlicingDice's API Docs are available at https://docs.slicingdice.com

Implementation Overview

feature

[cygnus-common] com.telefonica.iot.cygnus.backends.slicingdice.SlicingDiceBackendImpl

  • This class extends HttpBackend.
  • createColumns()
    • This method will create all the necessary SlicingDice columns if the user doesn't specify the auto_create=true configuration.
  • insertContextData()
    • This method stores the data on SlicingDice servers using the REST API.

[cygnus-ngsi] com.telefonica.iot.cygnus.sinks.NGSISlicingDiceSink

  • This class extends NGSISink.
  • The configurable variables are below:
variable default description
database_key the database key used to connect to SlicingDice
auto_create true if true we will create the columns automatically on SlicingDice as we insert

Dockerfile, agent.conf and cygnus-entrypoint.sh

  • Dockerfile, agent.conf and cygnus-entrypoint.sh are edited to be able to use NGSISlicingDiceSink.

@@ -9,3 +9,4 @@
[cygnus-ngsi][KafkaSink] Using global connection to zookeeper instead of creating one each time an event arrives
[cygnus-ngsi][NGSINameMappingsInterceptor] Now namemapping checks sevice, subervice and (type of entity and id entity) of EntityMapping (#1535)
[cygnus-ngsi][NGSIEvent] Unable to deliver event: null pointer getAttributeForNaming (#1506)
[cygnus-ngsi][SlicingDiceSink] Added SlicingDice sink to cygnus
Copy link
Member

Choose a reason for hiding this comment

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

Issue number should be cited at the end of the line (check other lines in the same file, please).

Small typo: cygnus -> Cygnus ;)

public class SlicingDiceBackendImplTest {

// constants
private static final String DATABASE_KEY = "oiasdiondasidasndasomn";
Copy link
Member

Choose a reason for hiding this comment

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

Fake key? (just to check)

* @param in
* @return The encoded string
*/
public static String encodeSlicingDice(String in) {
Copy link
Member

Choose a reason for hiding this comment

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

97, 119, 121, 122, 48, 57...

They may appera as magic number for people not very familiar with encoding. Maybe it would be a good idea to include a little explanation on how this function works in the JavaDoc preamble (explaining the difreent ranges/exceptions).


public static class PersistBatchTest {

private static final String DATABASE_KEY = "oiasdiondasidasndasomn";
Copy link
Member

Choose a reason for hiding this comment

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

Fake key? (just checking)

NGSISlicingDiceSink sink = new NGSISlicingDiceSink();
sink.setPersistenceBackend(mockBackend);

final String databaseKey = "oasdisadnasoi";
Copy link
Member

Choose a reason for hiding this comment

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

Fake key? (just checking)

@@ -139,6 +141,20 @@ ENV CYGNUS_POSTGRESQL_BATCH_TIMEOUT ""
ENV CYGNUS_POSTGRESQL_BATCH_TTL ""
ENV CYGNUS_POSTGRESQL_ENABLE_CACHE ""

# SlicingDice options
Copy link
Member

Choose a reason for hiding this comment

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

New env vars should be documented in the proper documentation file (more on this in a general comment I'll provide soon).

@@ -180,6 +182,19 @@ cygnus-ngsi.sinks.orion-sink.keystone_ssl = false
cygnus-ngsi.sinks.orion-sink.orion_fiware =
cygnus-ngsi.sinks.orion-sink.orion_fiware_path =


# SlicingDiceSink configuration
Copy link
Member

Choose a reason for hiding this comment

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

New configuration parameter (i.e. cygnus-ngsi.sinks.slicingdice*) should be documented in the proper documentation file (more on this in a general comment I'll provide soon).

@fgalan
Copy link
Member

fgalan commented Nov 26, 2018

Thank you very much for the contribution!

I have had a look to the PR. With regards to code I haven't performed an in deep review (I'm afraid I lack the knowledge in SliceDice to do that ;). I have only look for kind of "formal" aspect, with some minor comments regarding it. As long as the new sink doesn't break the existing e2e regression (we would check upon merging), that's ok.

My main piece of feedback is regarding documentation. The work you have done needs to be properly and comprehensively documented in the PR along with the code and unit test contribution. This includes:

  • Main README.md, to include SliceDice in the list of supported sinks (i.e. a new bullet in "NGSI-like context data in:" list)
  • Add a new file slicedice_backend.md to doc/cygnus-common/backends_catalogue/ directory, properly linked from the README.md in that same directory. Please, have a look to the other files in the same directory and use the same content structure.
  • Add a new file slicedice_sink.md to doc/cygnus-ngsi/flume_extensions_catalogue/ directory, properly linked from the README.md in that same directory ("Sinks" section). Please, have a look to the other xxx_sink.md files in the same directory and use the same content structure. This is the place where the table with cygnus-ngsi.sinks.slicingdice* parameters is provided, in addition to examples.
  • Modify doc/cygnus-ngsi/installation_and_administration_guide/install_with_docker.md to include the env vars used by Dockerfile/cygnus-entrypoint.sh and their description. Withing "Using a specific configuration" add a new bullet for your sink.
  • Add the new files to mkdocs.yml so they can be also rendered in the RTD version of the documentation (https://fiware-cygnus.readthedocs.io/en/master/)

This list could not be exahustive. It would be great if you could have a look to the Cygnus .md files in general in the repository to find any other possible directory/file that could need additions and I have missed, please.

Thanks!

@gmenegatti
Copy link

Hi @fgalan,

Thanks so much for the review and all the comments.

We will correct everything ASAP and let you know.

Regards,
Gabriel.

@fgalan
Copy link
Member

fgalan commented May 30, 2019

@joaosimbiose any update in this PR, please? Don't hesitate of asking if something is not clear about what is pending in this PR to be finished and merged. Thanks!

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.

3 participants