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

Add support for Cached Queries #102

Closed
josephwegner opened this Issue Jan 19, 2017 · 11 comments

Comments

3 participants
@josephwegner
Copy link
Contributor

josephwegner commented Jan 19, 2017

MVP

  • Create Saved Query
  • Cache a Saved Query
  • Uncache a Saved Query
  • Delete a Saved Query
  • Retrieve Results from Saved Query

Stretch

  • Retrieve definition of a saved query
  • Update a cached query
@josephwegner

This comment has been minimized.

Copy link
Contributor Author

josephwegner commented Jan 19, 2017

I think technically we can do all of the caching pieces right now, but it is hard. I'd like to get some more direct helper functions added, like keen.saved_queries.cache(name, interval). We also need to add docs to the README for it.

@tbarn tbarn added the hacktoberfest label Sep 28, 2017

@myrridin

This comment has been minimized.

Copy link
Contributor

myrridin commented Oct 5, 2017

I've run into some hiccups on this issue, and I'd like to ask a few clarifying questions.

First, what I've noticed so far

  • Saved query creation was not working with a hash of attributes, as the hash was not being encoded to a string before being passed to the HTTP put. This is done in other places and seems to be the expected behavior.
  • Updates do not work the way the documentation suggests. Update is currently just an alias for create, and seems to require a complete query definition, not just a list of fields to update.
  • The API's update method documentation states that a full query definition is required, though the example code shows a partial set of fields in most cases.

Questions:

  • Did the behavior of the update endpoint change at some point? The SDK docs have incremental changes for all supported SDKs, though the cURL examples uses a full definition.
  • Should I try to build an update method that fetches the current definition and attempts to merge a subset of values? This could provide a method that behaves more like the gem docs suggest, but might be inconsistent with the way the API endpoint would act.

I can fix the creation issue easily enough, but the docs are inconsistent enough with regards to the update endpoint that I'm not sure whether I should be building a new method or updating the gem docs (and suggesting that the Keen.io docs get updated).

If I can figure out these questions, I'm happy to build out this set of methods and update the docs to be complete and accurate.

@tbarn

This comment has been minimized.

Copy link
Contributor

tbarn commented Oct 5, 2017

@myrridin good questions! I used to be more familiar with this code, but I need to do some digging first. This is what I do know:

On the API level, the API reference is correct when it says:

To change the definition of the query, you must provide a complete new definition for the query in the "query" field in the body.
To rename a query, simply provide a "query_name" field in the request body with the new name. Note, this changes the URL for all requests going forward.

It's not the most elegant "update," but the SDK might be able to make it a bit easier to deal with. I know we have talked about this for other SDKs, but I am not sure which ones have it implemented.

Let me look into a few things and get back to you with better answers for your questions.

@tbarn

This comment has been minimized.

Copy link
Contributor

tbarn commented Oct 5, 2017

Another note before I have more answers, the way the Python client works with Saved and Cached Queries is nice: https://github.com/keenlabs/KeenClient-Python#saved-queries

@myrridin

This comment has been minimized.

Copy link
Contributor

myrridin commented Oct 5, 2017

I think it would make a lot of sense to mimic what the Python client is doing. After a quick review, it makes a lot of sense to provide both options. I'll definitely dig into this deeper once I'm back to open source time.

@tbarn

This comment has been minimized.

Copy link
Contributor

tbarn commented Oct 5, 2017

I'm starting to think that some things were just incorrectly documented or implemented in the gem, but I have sent a few messages to see if I can get to the bottom of it for more context.

@myrridin

This comment has been minimized.

Copy link
Contributor

myrridin commented Oct 5, 2017

My expectation is that the README.md documentation is incorrect, and full updates are required from the API's point of view. This should not be too difficult to abstract away in the SDK, then the docs can be updated to reflect the actual behavior (and the new behavior of update_full, mimicking the python lib).

One of the difficulties here is writing tests. The integration test suite seems to depend on a certain set of data, and does not seem to seed that data from what I can tell. It's obvious why the keys for your integration project are no longer part of the repo, but finding a way to make running and writing integration tests a little easier for contributors could go a long way towards the stability of actual API interactions.

I think it might be good to have a helper that can clear out a project, and seed appropriate data for the integration suite. I have to think about this more, but once I've written this functionality I'm not sure I'll be able to thoroughly evaluate it outside of an IRB session. Given that neither create nor update are currently working it's probably not a huge problem, but I'd love to have a way to ensure this is working properly going forward.

I appreciate you sending along the questions. I think there's a lot of opportunity to enhance this area of the SDK, and perhaps even the overall contributor experience.

@tbarn

This comment has been minimized.

Copy link
Contributor

tbarn commented Oct 9, 2017

Still working on this! Hopefully, will have a better response tomorrow.

@myrridin

This comment has been minimized.

Copy link
Contributor

myrridin commented Oct 9, 2017

No worries. I was at a hackathon all weekend anyway :-) I'm happy to be patient. Sounds like this is a complex discrepancy to figure out.

@tbarn

This comment has been minimized.

Copy link
Contributor

tbarn commented Oct 17, 2017

Hey @myrridin - sorry it has taken so long to get back to you!

It looks like all of your assumptions in the first paragraph are correct.

As for tests, I see what you mean about how it is looking for a specific set of data yet isn't creating it for you. Definitely not ideal. Sadly, I am not even sure what that set of data is anymore. I'm going to do some digging to see if I can find it, but it looks like it has been a while.

Is there anything I haven't touched on that is blocking you? I'll focus on the most important first.

@myrridin

This comment has been minimized.

Copy link
Contributor

myrridin commented Oct 22, 2017

PR has been submitted :-)

#127

Please let me know if you have any questions.

@tbarn tbarn closed this Nov 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.