vapor-ware / synse-client-go Goto Github PK

As @edaniszewski suggested in #1, it's a good idea to have different constructors for clients since it gives us more flexibility, like so:

// Creates a new synse client, does not specify API version.
func NewSynseClient() (*Client, error)

// Creates a new synse client with API v3
func NewSynseV3Client() (*Client, error)

WebSocket streamed reads have been added, but the initial implementation, while functional, feels a bit messy. This issue is to eventually go back and revisit the implementation to see if it can be cleaned up, both from a user experience perspective and from an implementation perspective.

In #3, its proposed that we could specify websocket v. http in a config option to the SynseClient.

A question I have is whether is make sense to do it this way, or if the semantics/usage of websockets are different enough from http to just make it its own client, e.g. related to #7

• NewHTTPClient
• NewHTTPClientV3
• NewWebsocketClient
• NewWebsocketClientV3

I'm not sure which makes the most sense, but I imagine that with a bit of research and some experimentation, it should become evident quickly

As of now, mock json responses are inline with their corresponding test functions. This is totally fine. However, these mock responses are very big sometimes and that make the test functions really long. If we can put them somewhere, ideally in test files, under a test data folder, that would be awesome.

As discussed in blackbox's PR 104, we could make the type conversion here.

As @edaniszewski suggested:

Instead of being one long hash string like 34c226b1afadaae5f172a4e1763fd1a6 it'll be a UUID string like 1b714cf2-cc56-5c36-9741-fd6a483b5f10

Before we try to send a close message, check that the WebSocket connection was started, otherwise calling Close will likely lead to a panic

Basically, need to omit system field in UnitOptions due to v3 amendment 2.

some command options do not have annotations

while other do

and some have a subset of what the others have (e.g. missing mapstructure annotation)

This issue is to normalize the annotations on command options. I'm fine with there being all annotations, some, or none -- whatever makes the most sense for them. Whichever way, I think they should just be consistent.

I think the answer to this may be yes. Since reading cached readings may return a lot of readings at once, that would be pretty large to put into memory all at once ([]Reading) just to iterate over one at time to process.

I'm not sure of the feasibility of this, but I suspect it should be doable. Maybe changing the interface to something like

ReadCache(chan<- *scheme.Read, scheme.ReadCacheOptions) error

in #70, according to @edaniszewski, ci has such failure on test:

+ CGO_ENABLED=0 go test -covermode=atomic ./...
go: disabling cache (/.cache/go-build) due to initialization failure: mkdir /.cache: permission denied
go: cannot use modules with build cache disabled
script returned exit code 1

@lazypower said it could be related to user permissions when using docker on host.

in looking through the models, I noticed one thing that seems incorrect:

in particular, the metadata should be a map[string]string, not its own type with expected fields -- the data in there is totally arbitrary.

I didn't notice anything else, but I'll take another pass through later and see if I notice anything.

work items for this issue:

• fix metadata in Info scheme
• add "alias" to info response (see: docs)
• add "alias" to scan response (see: docs)
• update Write scheme (Transaction -> Id) (see: docs)

Based on #29 suggestion, we should use RFC3339 for timestamp value instead of RFC3339Nano across all mock tests.

creasty/defaults is recommended by @edaniszewski in #1 comment in order to complete this. Need #3 so we can leverage it better.

while doing dev work for synse v3 in the synse-cli, I noticed that in some cases, yaml formatting can be different than json formatting, e.g. (see the version section):

JSON

[
  {
    "active": true,
    "id": "4032ffbe-80db-5aa5-b794-f35c88dff85c",
    "name": "emulator plugin",
    "description": "A plugin with emulated devices and data",
    "maintainer": "vaporio",
    "tag": "vaporio/emulator-plugin",
    "vcs": "",
    "version": {
      "plugin_version": "",
      "sdk_version": "",
      "build_date": "",
      "git_commit": "",
      "git_tag": "",
      "arch": "",
      "os": ""
    }
  }
]

YAML

- active: true
  id: 4032ffbe-80db-5aa5-b794-f35c88dff85c
  name: emulator plugin
  description: A plugin with emulated devices and data
  maintainer: vaporio
  tag: vaporio/emulator-plugin
  vcs: ""
  version:
    pluginversion: ""
    sdkversion: ""
    builddate: ""
    gitcommit: ""
    gittag: ""
    arch: ""
    os: ""

This issue is to add annotations to the scheme types so yaml output will have the same keys as json output

refer to vapor-ware/synse-server#335

• http client
• websocket client

Follow-up on #79, below are the logs for Synse Server 500 responses for websocket requests:

scan

synse-server    | timestamp='2019-09-05T02:52:19.720950Z' level='debug' event='processing websocket request' handler='handle_request_scan'
synse-server    | timestamp='2019-09-05T02:52:19.721196Z' level='error' event='error processing request' err=TypeError("object of type 'NoneType' has no len()",)
synse-server    | timestamp='2019-09-05T02:52:19.729562Z' level='debug' event='stopping message handler for websocket' host=None

tags

synse-server    | timestamp='2019-09-05T02:54:35.514455Z' level='debug' event='processing websocket request' handler='handle_request_tags'
synse-server    | timestamp='2019-09-05T02:54:35.514598Z' level='error' event='error processing request' err=TypeError('tags() argument after * must be an iterable, not NoneType',)
synse-server    | timestamp='2019-09-05T02:54:35.523650Z' level='debug' event='stopping message handler for websocket' host=None

read

synse-server    | timestamp='2019-09-05T02:55:45.324884Z' level='debug' event='processing websocket request' handler='handle_request_read'
synse-server    | timestamp='2019-09-05T02:55:45.325572Z' level='error' event='error processing request' err=TypeError("object of type 'NoneType' has no len()",)
synse-server    | timestamp='2019-09-05T02:55:45.341372Z' level='debug' event='stopping message handler for websocket' host=None

write async

synse-server    | timestamp='2019-09-05T02:56:58.861106Z' level='debug' event='caching transaction' plugin='4032ffbe-80db-5aa5-b794-f35c88dff85c' id='eea7a761-ef56-4a86-8ccd-d862da5dba05' device='f041883c-cf87-55d7-a978-3d3103836412'
synse-server    | timestamp='2019-09-05T02:56:58.862166Z' level='debug' event='caching transaction' plugin='4032ffbe-80db-5aa5-b794-f35c88dff85c' id='f1b803a0-e7b4-441b-b4f5-04bdef14712a' device='f041883c-cf87-55d7-a978-3d3103836412'
synse-server    | timestamp='2019-09-05T02:56:58.863292Z' level='error' event='error processing request' err=TypeError("Object of type 'bytes' is not JSON serializable",)
synse-server    | timestamp='2019-09-05T02:56:58.875558Z' level='debug' event='stopping message handler for websocket' host=None

write sync

synse-server    | timestamp='2019-09-05T02:59:18.471559Z' level='debug' event='caching transaction' plugin='4032ffbe-80db-5aa5-b794-f35c88dff85c' id='dc692e5a-5ed7-46db-ba01-b2ddfc05f984' device='f041883c-cf87-55d7-a978-3d3103836412'
synse-server    | timestamp='2019-09-05T02:59:18.473183Z' level='debug' event='caching transaction' plugin='4032ffbe-80db-5aa5-b794-f35c88dff85c' id='7a14867d-37fc-4565-87e1-de1afadbf61b' device='f041883c-cf87-55d7-a978-3d3103836412'
synse-server    | timestamp='2019-09-05T02:59:18.474618Z' level='error' event='error processing request' err=TypeError("Object of type 'bytes' is not JSON serializable",)
synse-server    | timestamp='2019-09-05T02:59:18.484667Z' level='debug' event='stopping message handler for websocket' host=None

@edaniszewski, let me know if that makes sense.

For the most part, a Websocket and a HTTP client expose similar sets of API methods. However, since they are different in their usage and semantics, there are some methods that are only applicable for one and not the other. In those cases, these methods will have no effect but just be there to fulfill the interface. The question is, does it make sense anymore keep that Client interface that all client should implement anymore?

Based on suggestion on #36, will need to adopt the new build pipeline with goreleaser for future improvement.

As mentioned in #20 comment, need to do more research on how to do this.

I thought this was done previously, but maybe it slipped through the cracks. The metrics scheme (synse/scheme/metrics.go) doesn't define anything and isn't being used. Metrics will be exported from synse server as prometheus data, so it wouldn't make sense to have a struct for it here anyways.

TBA once v3 proposal is finalized. However, it’s likely that v3 will support websocket.

Information on websocket api for v3 can be found here.

This include:

• Updating docstrings to be more descriptive
• Adding links for reference if needed.
• Filling in README and any additional docs on how to use.
• Adding more examples of usage.

vapor-ware/synse-server#281

there are a bunch of API calls which return *[]Type which I think should instead return []*Type -- this makes it easier to consume, as you don't have to deference the list pointer prior to using it, e.g.

response, err := client.Read()
if err != nil {
    return err
}

if len(*response) == 0 {
    return nil
}

for _, r := range *response {
    // do something
}

would become

response, err := client.Read()
if err != nil {
    return err
}

if len(response) == 0 {
    return nil
}

for _, r := range response {
    // do something
}

its a small/subtle difference, but does generally make things a bit easier to handle.

#12 (comment)

In the above PR, I noticed a few places where version caching was being done. While that idea has been around for a while and in some degree makes sense and is a nice feature, I think we should discuss its usefulness here/the assumptions built in around it. I think that we may be able to simplify a bunch of that stuff, but I'd like to discuss and pick your brain first, @hoanhan101.

Due to #23 suggestion, it would make more sense to test each api function in its own test case instead of grouping all of them in a table test.

At the moment, what we have for TLS config options are:

// TLSOptions is the config options for TLS/SSL communication.
type TLSOptions struct {
	// CertFile and KeyFile are public/private key pair from a pair of files to
	// use when communicating with Synse Server.
	CertFile string `default:"-"`
	KeyFile  string `default:"-"`

	// Enabled specifies whethere tls is enabled.
	Enabled bool `default:"false"`

	// SkipVerify specifies whether the client can skip certificate checks.
	SkipVerify bool `default:"false"`
}

Even though SkipVerify should only be used for testing, I am not sure if it's a good idea to leave it exported/configurable like so. Also, linting doesn't like that either.

TBA once v3 proposal is finalized. One thing I know for sure at the moment is that the client library that we are using, go-resty/resty, supports basic authentication and tls configs.

refer to vapor-ware/synse-server/issues/329

TBA once v3 proposal is finalized. Some configurable options are:

• Synse Server’s address
• Request timeout
• Retry backoff strategy
• Transport: http, websocket
• TLS options