Introduce Split Registry Endpoint Accepting a Timestamp #133
Conversation
|
@phantomphildius isn't a platform reviewer and needs to request platform review. |
|
/platform @aburgel @jmileham @smudge @samandmoore |
|
Needs somebody from @Betterment/test_track_core to claim domain review Use the shovel operator to claim, e.g.:
|
|
nanda |
|
Needs somebody from @Betterment/test_track_core to claim domain review Use the shovel operator to claim, e.g.:
|
| @split_registry = SplitRegistry.instance | ||
| build_timestamp = BuildTimestamp.new(build_params) | ||
| if build_timestamp.valid? | ||
| @split_registry = SplitRegistry.new(build_timestamp.build_timestamp) |
There was a problem hiding this comment.
Seeing this all live makes me want to rethink our approach a bit. The relationship between timestamp and split registry feels a little tenuous. It's not immediately clear from the API why you would be passing in a timestamp, and what kind of output that would lead to.
We had toyed with the idea of using a different name, something like split_registries_snapshot to represent the (quasi-)point-in-time nature of the results we're returning.
If we went down that route, we'd keep the original route, add a new one with the above name, and then combine BuildTimestamp and SplitRegistry model into a new one called SplitRegistrySnapshot that would take in the timestamp.
With this approach, there's a clear relationship between the endpoint name and the param it accepts. And it allows us to have a model that encapsulates all of this.
What do you think?
cc @smudge @samandmoore would love your thoughts too!
There was a problem hiding this comment.
Interesting. This makes me think that the contract between apps isn't entirely clear -- if it isn't too late, I'd be inclined to propose renaming timestamp to as_of, so that the semantics of asking for a "split registry" make sense when you tag on "?as_of=XXYYZZ" -- adding "snapshot" to the name then feels a little redundant in that world. (but, does feel necessary when it isn't clear what "timestamp" means or does)
There was a problem hiding this comment.
Yeah, as_of would make it clear within the code, but the route would still be ambiguous:
/api/v2/split_registries/2019-12-13T00:00:00
vs
/api/v2/split_registry_snapshots/2019-12-13T00:00:00
I suppose if we had documentation, we could give the parameter a name (like as_of), but it would be nice if the url was somewhat self-documenting.
There was a problem hiding this comment.
Just reread your comment. It makes sense if we use a query param, but since this is a required param, we're making it part of the path.
There was a problem hiding this comment.
Ah, ok -- I guess I should've looked closer at the routes 😅 -- so if we're committed to putting it in the path then we can't name it, and if we can't name it, then yeah that forces design considerations around the resource name.
But it's not exactly a "snapshot," right? It's a lookup of the most recent state prior to the timestamp (as in, not an exact match, like with a true ID lookup). IMO for other query/search-ey endpoints, it's fine to have a required param that lives in the technically optional query param list -- you just return a 5XX error if it's missing.
e.g. /search?q=test+123
I get that this isn't actually a search endpoint, but it's also not pulling up a "snapshot" with an exact match on the ID param -- it's an "as of" query.
Another interesting thing is how this is gonna appear in the logs/APM/etc -- typically the set of possible IDs is somewhat bounded, but in this case the "ID" is a timestamp, which means it'll be like making a bunch of web requests to resource/:id with ever-increasing ids that mostly never overlap in the same way that DB ids would. So you'd better hope that the APM dashboard is good at pattern matching and turning it into a wildcard! I'd assume it would, but I've also never tried it with a formatted timestamp before... 🤔
There was a problem hiding this comment.
Also, sorry for the wall of text, I'm just trying to get all of my thoughts down in one place! If we end up going with a resource name change -- i.e. /api/v2/split_registry_snapshots/2019-12-13T00:00:00 -- instead of pulling it out into a named param, my only thought would be to double check and make sure our APM solution can handle timestamps as resource IDs.
There was a problem hiding this comment.
my only thought would be to double check and make sure our APM solution can handle timestamps as resource IDs.
We already do this for the app_visitor_configs route, so I'm not worried about adding another similar case. It looks good in the APM too.
IMO for other query/search-ey endpoints, it's fine to have a required param that lives in the technically optional query param list -- you just return a 5XX error if it's missing.
Yeah, I don't have a huge problem with required query params, though I think search might be the only great example of this. In this case, the latest refactor shows the value of treating this like a standard REST resource with an identifier parameter. It provides a single encapsulated model, and there's nice alignment between the route and model name.
I'm with you that it's stretching the notion of "snapshot". I'm open to other names if you have any 😄.
My defense of "snapshot" is that it serves that from the client's perspective. Its a (mostly) consistent result that gives the client a point in time representation of the split registry. Splits weights will change and we will ship newly created splits. Clients will ignore newly created splits, and the updated weight... well, that's how TT works. 🤷♂
There was a problem hiding this comment.
/api/v2/builds/[timestamp]/split_registry seems like it'd be the most coherent with the mobile route. We did a lot of this debate before settling on the URL structure of the mobile route - we could reopen it, but we def talked about optional vs required query params and naming of things, etc.
After discussion we decided to rename this endpoint and fold the build_timestamp validation logic into SplitRegistry. Besides making this more of a true resource route, it explains what this endpoint is retunring much better, and consolidates code. Now the SplitRegistrySnapshot class accepts a timestamp and returns to you the split registry as of that time. So this change will be backwards compatifble as we just need to pass it `Time.zone.now`
|
that works for me! |
|
nanda |
|
Needs somebody from @Betterment/test_track_core to claim domain review Use the shovel operator to claim, e.g.:
|
|
nanda |
|
Needs somebody from @Betterment/test_track_core to claim domain review Use the shovel operator to claim, e.g.:
|
|
<< domain LGTM - moving this to the |
|
Needs somebody from @aburgel, @jmileham, @smudge, and @samandmoore to claim platform review Use the shovel operator to claim, e.g.:
|
|
nanda bump |
|
Needs somebody from @aburgel, @jmileham, @smudge, and @samandmoore to claim platform review Use the shovel operator to claim, e.g.:
|
| @@ -0,0 +1,37 @@ | |||
| class SplitRegistrySnapshot | |||
There was a problem hiding this comment.
given all the other changes, snapshot no longer makes much sense, let's just change this back to SplitRegistry.
| with: /\A\d{4}-[01]\d-[0-3]\dT[0-2]\d:[0-5]\d:[0-5]\d(.\d+)?([+-][0-2]\d:[0-5]\d|Z)\z/, allow_blank: true | ||
| } | ||
|
|
||
| def initialize(timestamp:) |
There was a problem hiding this comment.
as_of? it lines up with the naming convention in Split and clarifies what the timestamp is for
There was a problem hiding this comment.
There still needs to be an initializer though, which probably needs this signature, right? .as_of could be added as a sugar class method tho? Or am I misinterpreting?
There was a problem hiding this comment.
sorry, my comment wasn't clear, i meant changing the arg name from timestamp to as_of.
There was a problem hiding this comment.
oh oh, i knew I was misunderstanding. :)
|
<< platform tafn |
|
@phantomphildius needs to incorporate feedback from @aburgel. Bump when done. |
|
bump |
|
Needs @aburgel to provide platform review When you finish a round of review, be sure to say you've finished or sign off on the PR, e.g.:
If you're too busy to review, unclaim the PR, e.g.:
|
config/routes.rb
Outdated
|
|
||
| namespace :v3 do | ||
| resources :builds, only: [], param: :timestamp do | ||
| resource :split_registry, only: :show, controller: 'split_registries' |
There was a problem hiding this comment.
i think specifying the controller is unnecessary. it should be able to infer that from the resource name
| get :show, params: { build_timestamp: '2019-11-14T14:35:30Z' } | ||
|
|
||
| expect(response).to have_http_status :ok | ||
| expect(response_json['splits']).to eq({}) |
There was a problem hiding this comment.
shouldn't splits two and three appear here? i think this might be caused by using let instead of let!
There was a problem hiding this comment.
ah yeah, i was relying on the expectation before executing the controller action to decide which splits I would persist for the test. Probably not the clearest for someone coming to this file for the first time. Updated using contexts.
spec/models/split_registry_spec.rb
Outdated
| split = FactoryBot.create(:split, finished_at: Time.zone.now) | ||
|
|
||
| expect(described_class.new(as_of: 1.day.ago).splits.all).to include(split) | ||
| end | ||
| end | ||
|
|
||
| describe "#experience_sampling_weight" do | ||
| context "bypassing singleton memoization" do |
There was a problem hiding this comment.
this context is no longer applicable since we aren't using a singleton anymore. can you remove?
|
platformlgtm! awesome work, thanks for getting it over the finish line. left a couple of minor comments, then it's good to land. |
|
Approved! 💰 👌 👍 |
| @@ -2,6 +2,6 @@ class Api::V2::SplitRegistriesController < UnauthenticatedApiController | |||
| include CorsSupport | |||
|
|
|||
| def show | |||
| @split_registry = SplitRegistry.instance | |||
| @split_registry = SplitRegistry.new(as_of: Time.zone.now) | |||
There was a problem hiding this comment.
Hey so given the fact that if I merge and deploy this, the behavior will be on my head I looked a little deeper at the current behavior here. It's my understanding that the closest to zero behavior change for this call would be to pass as_of: nil, because instance used to mean Split.for_presentation(), and Split.for_presentation() would call Split.active() which would apply the condition where(finished_at: nil). As it is this would end up applying the condition where('splits.finished_at is null or splits.finished_at > ?', Time.zone.now), which is sort of spurious since we'd expect there to be no splits finished in the future, but clock error and all that jazz could make that a thing under some circumstances.
Granted this would fail validation for SplitRegistry, if we actually valid?, which we're not doing here. Do we care? I guess not. Ok I'm on board with this. Basically this is behaviorally correct under all normal circumstances and hard to behaviorally regress, and the whole v2 api is soft deprecated at this point anyway so the blast radius of a regression here is low. Ok. I'mma merge this.
Summary
Deprecates the current split registry endpoint (by renaming to v2/legacy_split_registry) and replaces with a new v2/split_registries that accepts a timestamp as a parameter. This will allow for finding the active splits as of that timestamp - rather than as of the current time.
/domain @Betterment/test_track_core