feat: Added multi root queries to the model #84
Conversation
autoagora/logs_db.py
Outdated
| hash: bytes | ||
| query: str | ||
| query_time_ms: int = 0 | ||
| timestamp: datetime = datetime.now() |
There was a problem hiding this comment.
The timestamp will end up being a constant set at the moment the program starts (when the class is created), not when a new instance of this class is created.
| model = await mrq_model_builder(subgraph, pgpool) | ||
| await set_cost_model(subgraph, model) | ||
| # Should this also be this amount of time? | ||
| await aio.sleep(args.relative_query_costs_refresh_interval) |
There was a problem hiding this comment.
This should be a different time, otherwise it would be too slow.
Also consider using random.lognormvariate: https://www.desmos.com/calculator/k3en6o5ikf
There was a problem hiding this comment.
What would be a good "default" value and apply lognormvariate to it, to give it some "randomness"?
There was a problem hiding this comment.
The values in the link should be a good start
autoagora/logs_db.py
Outdated
| @@ -52,7 +179,7 @@ async def get_most_frequent_queries( | |||
| Avg(query_time_ms) as avg_time, | |||
| Stddev(query_time_ms) as stddev_time | |||
| FROM | |||
| query_logs | |||
| {query_logs_table} | |||
There was a problem hiding this comment.
@aasseman So this part, didn't let me to either leave it as %s and insert values or using $1, since the syntax doesn't work if the variable is alone like that.
Had to add format to it and use it this way, however this is vulnerable and SQL injections this way are a possibility, although this variable is not accesible by the user, what do you think?
Should I then create a separate query so that we don't have that there?
There was a problem hiding this comment.
Yeah, it's not great. There is a SQL solution mentionned here. It also mentions that psycopg2 has a solution for this, but it's not async.
However, it seems that psycopg3 came out recently, and it supports async! Not sure if it's worth/hard porting to it though.
There was a problem hiding this comment.
So looked further into this, the easiest quickest approach is to just separate the function into two or to have in the constanst file the query already defined and select the query depending the type of query its looking for , which should be the one that leaves the code the "cleanest"
Signed-off-by: carlos.vdr <[email protected]>
Signed-off-by: carlos.vdr <[email protected]>
Signed-off-by: carlos.vdr <[email protected]>
Signed-off-by: carlos.vdr <[email protected]>
Signed-off-by: carlos.vdr <[email protected]>
Signed-off-by: carlos.vdr <[email protected]>
Signed-off-by: carlos.vdr <[email protected]>
Signed-off-by: carlos.vdr <[email protected]>
Signed-off-by: carlos.vdr <[email protected]>
Signed-off-by: carlos.vdr <[email protected]>
carlosvdr
force-pushed
the
multi-root-q
branch
from
bc2b0bb
to
7997689
Compare
Signed-off-by: carlos.vdr <[email protected]>
Pull Request Test Coverage Report for Build 5686688707Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
1 similar comment
Pull Request Test Coverage Report for Build 5686688707Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Issue: #43