rackspace / gophercloud

A Go SDK for OpenStack. IN FEATURE FREEZE. See Issue #592
http://gophercloud.io
Other
456 stars 180 forks source link

[WIP] - Metrics Integration with GopherCloud. #503

Closed goru97 closed 8 years ago

goru97 commented 8 years ago

What: Adding metrics support in gophercloud.

Why: On-demand from our customers.

How: By writing code in GOLang.

Additional Info: 100% test coverage.

goru97 commented 8 years ago

@jrperritt Am I doing this the right way?

rgbkrk commented 8 years ago

Really glad to see this coming along. The Travis test failure on go tip is unrelated (only on RackConnect).

jrperritt commented 8 years ago

@goru97 Not quite, but you're definitely on the right track, and the unit test coverage looks fantastic. Have a look at the List function here to see how we pass optional parameters to functions and then build the query string and HTTP headers. For example, the functions in urls.go should just return the base URL (sans query string).

goru97 commented 8 years ago

@jrperritt Thanks for your feedback. I'll work on it :)

goru97 commented 8 years ago

@jrperritt @rgbkrk Done some changes with the query builder; Also added the code for ingestion-api. Please take a look at your convenience.

rgbkrk commented 8 years ago

Yes, thank you so much for the structs. I'll leave @jrperritt to the rest.

goru97 commented 8 years ago

@rgbkrk Thank you for your patience and guidance :blush:

sam-falvo commented 8 years ago

One final, parting thought from me: make sure you run "go fmt" over your code. I've noticed some odd code formatting which go fmt will automatically fix for you.

goru97 commented 8 years ago

@jrperritt Do I need to write acceptance tests as well or is it considered done?

jrperritt commented 8 years ago

@goru97 Yes, you'll need to write acceptance tests, but I'd recommend waiting until the API is set before doing that. I'll go through today and point out some more things in the PR that need changing.

jrperritt commented 8 years ago

@goru97 I mentioned a few basic changes to make. Again, I'd encourage you go through the other Gophercloud packages and modify this PR to follow the style/naming/logic conventions shown there. Unrelated to that, I'm not crazy about the package layout here. That's no fault of yours; you've organized it as it's shown in the docs. But it doesn't read particularly well when being coded, and therefore doesn't conform to the Gophercloud style. Off the top of my head, instead of query and ingestion, I'm leaning toward 4 subpackages: metrics, counters, gauges, timers with the appropriate functions/objects in each. With that layout, we'd have metrics.List, metrics.ListAvailable, counters.Get, metrics.Limits, timers.Send etc. Maybe SendAggregated/GetAggregated, I'm unsure if there's a difference.

goru97 commented 8 years ago

@jrperritt I understand your concern about the package structure but changing it to 4 sub packages will make it worse because there are two different endpoints for metrics (one for ingest and other for query) and that's why for query functions, one has to pass ServiceClient for query endpoint and similarly for ingestion. But if we change it to metrics, counters, gauges... one has to pass ServiceClient for query endpoint while doing e.g. counters.send() and ServiceClient for ingest while doing counters.get(), which will only increase the complexity and also the confusion.

jrperritt commented 8 years ago

@goru97 You have a single NewMetricsv2 function for getting a Cloud Metrics client. What would a user provide differently to that function to get the different endpoints? Do you have an example of what the Cloud Metrics entry looks like in the catalog?

goru97 commented 8 years ago

@jrperritt Here's the gist for making service clients. Notice the service name passed in the endpointOpts. https://gist.github.com/goru97/77da071cf930230d4cd1

jrperritt commented 8 years ago

If there are 2 different endpoints, there should be 2 different clients and 2 different packages beneath the rackspace package.

goru97 commented 8 years ago

@jrperritt Did you mean one metricsQuery and other metricsIngest?!!

jrperritt commented 8 years ago

Yes, but all lowercase.

jrperritt commented 8 years ago

Closing due to lack of activity