intelsdi-x / snap

The open telemetry framework
http://snap-telemetry.io
Apache License 2.0
1.8k stars 294 forks source link

Addition and Refactor Snap Restful APIs #1547

Open candysmurf opened 7 years ago

candysmurf commented 7 years ago

In order to become compliant with OpenAPI Specification standard (formally known as Swagger) and removing business logic from the client, refactoring and/or addition to the following Snap APIs is necessary.

Adding new rest APIs

Current State

Those CLIs have no rest APIs. Business logic is mixed inside CLIs

Future State

Creating new Rest APIs and corresponding CLIs for the following:

Enhancing create task rest API

Current State

CLI defines task struct type which is out sync with Snap Task type already. Mainly two issues

Future State

Adding new Rest APIs for authentication and SSL

Current State

Snap has CLIs to do encryption/decryption and user authentication but it has no corresponding RestAPIs.

Future State

Snap supports both RestAPI and CLI for authentication and SSL.

Adding CLIs for updating configs

Current State

Snap has Rest APIs for updating/deleting configs. There is no corresponding CLIs.

Future State

Snap supports CLIs for updating/deleting configs

andrzej-k commented 7 years ago

Hi @candysmurf. I'd have few questions:

  1. For new APIs could you provide more detailed description, ie. what is a running plugin, and what does it mean expose a task?
  2. Could you also provide more details, like method type, example request and response to all new APIs?
  3. Could you expand more on new Rest APIs for authentication and SSL - how new API suppose to look like?
  4. Is API for updating and deleting config documented? Is this for global or task config? Will config update trigger snapteld restart? Can we design it to avoid it, especially if only plugin config is changed?
  5. For task create are we also going to support duration param? Or are we going to remove it from snaptel? --duration value, -d value The amount of time to run the task [appends to start or creates a start time before a stop]
candysmurf commented 7 years ago

@andrzej-k, thanks for your good questions. Please see my reply below:

  1. Currently, we have CLIs to show

    • a list of running plugins.
    • export a task
    • swap plugins The implementation for all them only have CLIs and no APIs. The problem is that they embed business logic inside CLIs. Every Snap client has to implement the same logic if Snap does not provide APIs.
  2. I cannot give you details in this reply. But I can before its implementation for your review and suggestions.

  3. Same reason as #1. Authentication and security only can be used in CLIs. The problem is that each Snap client will have to implement its own. The formality is same as the Restful API. e.g. http://127.0.0.1:8181/login that would require user/password.

  4. It's for global plugin config items. We have APIs for doing update/delete global plugin config items but have no CLIs. If these two operations are not useful, we shouldn't have exposed APIs either. Please share your thoughts.

  5. good question for this one. I have never used those parameters myself and am not sure how useful they're. I tried to bring the parity atm and didn't think of removing them. Please share your thoughts and suggestions.