optional "signature" argument for TCBS model

RESOLVED WONTFIX

Status

RESOLVED WONTFIX
5 years ago
3 years ago

People

(Reporter: rhelmer, Assigned: rhelmer)

Tracking

unspecified
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Assignee)

Description

5 years ago
For bug 915373 and bug 913266, it would be really handy to have a way to tell the TCBS model (and underlying mware service) a list of the signatures that should be returned.

The query still needs to run over all signatures in the requested date range though, in order to calculate the relative ranking.
(Assignee)

Comment 1

5 years ago
I don't think we want this on second thought - we'll probably get a better cache hit leaving it alone. Will measure and reopen if it seems useful.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
If we genuinely need TCBS *by signature* then I think we should have this functionality. Relying on caching is too weak IMHO. Also, the TCBS takes `days` and a `limit` so what if you do::

 api= TCBS()
 result = api.get(product=foo, version=bar, limit=10, days=100)
 relevant = [x for x in result if x['signature'] == my_signature]

Then you might be missing out on relevant results that are "masked" by the limit if you catch my drift.
(Assignee)

Comment 3

5 years ago
(In reply to Peter Bengtsson [:peterbe] from comment #2)
> If we genuinely need TCBS *by signature* then I think we should have this
> functionality. Relying on caching is too weak IMHO.

Agreed, reopening - thanks for digging this up.

> Also, the TCBS takes
> `days` and a `limit` so what if you do::
> 
>  api= TCBS()
>  result = api.get(product=foo, version=bar, limit=10, days=100)
>  relevant = [x for x in result if x['signature'] == my_signature]
> 
> Then you might be missing out on relevant results that are "masked" by the
> limit if you catch my drift.

True.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(Assignee)

Comment 4

5 years ago
(In reply to Peter Bengtsson [:peterbe] from comment #2)
> If we genuinely need TCBS *by signature* then I think we should have this
> functionality. Relying on caching is too weak IMHO.

So my thinking behind WONTFIXing this was that we could move the backend service to use matviews instead of doing ranking on-demand like this, since it's slow. This would be per-product+version - we could still do this but I think adding the "signature" dimension into the mix would be prohibitively complex.

I don't know if restricting signatures will actually make the query faster, it's worth measuring as I think it just might.
(In reply to Robert Helmer [:rhelmer] from comment #4)
> (In reply to Peter Bengtsson [:peterbe] from comment #2)
> > If we genuinely need TCBS *by signature* then I think we should have this
> > functionality. Relying on caching is too weak IMHO.
> 
> So my thinking behind WONTFIXing this was that we could move the backend
> service to use matviews instead of doing ranking on-demand like this, since
> it's slow. This would be per-product+version - we could still do this but I
> think adding the "signature" dimension into the mix would be prohibitively
> complex.
> 
Not sure I understand that second sentence. You think adding signature as a optional parameter to tcbs is "prohibitively complex"? Or to add a matview is prohibitively complex?

> I don't know if restricting signatures will actually make the query faster,
> it's worth measuring as I think it just might.

Should be easy to benchmark. You mean, SQL versus being able to memcache all TCBS locallly for each signature. 
However, there's still the point I made above about using the data "correctly" with the limit and stuff.
(Assignee)

Comment 6

5 years ago
(In reply to Peter Bengtsson [:peterbe] from comment #5)
> (In reply to Robert Helmer [:rhelmer] from comment #4)
> > (In reply to Peter Bengtsson [:peterbe] from comment #2)
> > > If we genuinely need TCBS *by signature* then I think we should have this
> > > functionality. Relying on caching is too weak IMHO.
> > 
> > So my thinking behind WONTFIXing this was that we could move the backend
> > service to use matviews instead of doing ranking on-demand like this, since
> > it's slow. This would be per-product+version - we could still do this but I
> > think adding the "signature" dimension into the mix would be prohibitively
> > complex.
> > 
> Not sure I understand that second sentence. You think adding signature as a
> optional parameter to tcbs is "prohibitively complex"? Or to add a matview
> is prohibitively complex?

I was thinking of the matview.

> 
> > I don't know if restricting signatures will actually make the query faster,
> > it's worth measuring as I think it just might.
> 
> Should be easy to benchmark. You mean, SQL versus being able to memcache all
> TCBS locallly for each signature. 
> However, there's still the point I made above about using the data
> "correctly" with the limit and stuff.

That makes sense for the current situation, but if you specify a list of signatures then you should only get that number of results back, for each product/version specified, so it should be possible to set it so this won't happen. Setting it to 0 might work to disable it too.
(Assignee)

Comment 7

5 years ago
Ugh, so due to the way this query works we cannot restrict by signature using a WHERE clause, since this is where it does the relative ranking: https://github.com/mozilla/socorro/blob/master/socorro/external/postgresql/tcbs.py#L70

Having the service limit the signatures it sends back to a list will require us to do a little less work and is probably worth it, but it's definitely not going to be any significant perf savings, unfortunately.
QA Contact: rhelmer
(Assignee)

Updated

5 years ago
Depends on: 957463

Comment 8

5 years ago
(In reply to Robert Helmer [:rhelmer] from comment #7)
> Ugh, so due to the way this query works we cannot restrict by signature
> using a WHERE clause, since this is where it does the relative ranking:
> https://github.com/mozilla/socorro/blob/master/socorro/external/postgresql/
> tcbs.py#L70

That's why the idea came up to store the TCBS rank in a matview so that this as well as topchangers could use that instead of recalculating all the time.
(Assignee)

Comment 9

5 years ago
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #8)
> (In reply to Robert Helmer [:rhelmer] from comment #7)
> > Ugh, so due to the way this query works we cannot restrict by signature
> > using a WHERE clause, since this is where it does the relative ranking:
> > https://github.com/mozilla/socorro/blob/master/socorro/external/postgresql/
> > tcbs.py#L70
> 
> That's why the idea came up to store the TCBS rank in a matview so that this
> as well as topchangers could use that instead of recalculating all the time.

Yes I think we should do this, but it's a larger project.
(Assignee)

Updated

5 years ago
Blocks: 952246
(Assignee)

Updated

5 years ago
Assignee: nobody → rhelmer
Target Milestone: --- → 73
(Assignee)

Comment 10

5 years ago
Had a chat w/ peterbe and we went full circle on this - due to the way this query works, the best we could do right now is filter the output from the service, we may as well stay the course and do this in django, the marginal savings in data transfer isn't worth complicating the service further.

The longer-term (hopefully q2 goal) vision is to pre-calculate TCBS rankings, probably via matview and stored procedure.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → WONTFIX
(Assignee)

Updated

3 years ago
QA Contact: rhelmer
You need to log in before you can comment on or make changes to this bug.