-
Notifications
You must be signed in to change notification settings - Fork 37
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
feat(api): initialize metric tracking #239
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Lee-W could you update the PR description with all the metrics you are tracking, please?
I would suggest to add it in detail
1b5622d
to
296a1e0
Compare
Deploying with Cloudflare Pages
|
296a1e0
to
2475064
Compare
2475064
to
dac6e74
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are there files like api.pickle, parquet files, csv files added as part of this PR?
dac6e74
to
73cea79
Compare
They're accidentally added. Removed them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks good to me. But some similar data get stored in Firestore so it feels like redundant data between Snowflake and Firestore. Also from the maintenance point of view if we use only one between Firestore and Snowflake that would be great.
Thats right. But this is a request from Steven for data team |
d1a0780
to
6f14159
Compare
Just rewrite it as airflow DAG as informed |
6f14159
to
c880e02
Compare
|
||
from airflow.decorators import dag, task | ||
|
||
METRICS_SNOWFLAKE_DB_USER = os.environ.get("METRICS_SNOWFLAKE_DB_USER") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we not get these from the snowflake connection of airflow instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, but I'm not sure whether we want. We'll need to install the provider simply to get the conn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we not use this operator to run the query: https://airflow.apache.org/docs/apache-airflow-providers-snowflake/stable/operators/snowflake.html#snowflakesqlapioperator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I am okay with current implementation for now but we could track this in future PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Let's make sure to add a section in this doc for this here before we merge this PR.
Just update the documentation. Will merge this PR now |
In this PR, we track "request id", "score", 'success" and "created_at" which can be used to calculate the number of traces (i.e. root runs = user requests) and another view will be created to group requst by success/failure, and the calculate Avg. correctness score, by day.
closes: #196
We do not have Snowflake access, and the Snowflake trial didn't work for me. Use SQLite as a POC. will change it to snowflake after we get the access