-
Notifications
You must be signed in to change notification settings - Fork 383
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!: db table for trace data retention policies #6703
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.
Amazing work here Roger.
Main question I have is I'm not grokking the need for the active
column if the foreign key relation onto the trace retention policies table is nullable.
), | ||
) | ||
op.create_index( | ||
"ix_projects_trace_retention_policy_id", |
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.
Tricky that we have to manually specify the index name in the migration. Is there a way to use the same auto-generated name?
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.
unfortunately I have not been able to get the batch_op to work without doing it this way
src/phoenix/db/facilitator.py
Outdated
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.
It looks like the expected behavior is that the env var only takes effect the very first time Phoenix is deployed, and afterward, the default retention period is locked in forever?
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.
yea, that's right. We don't want to change it based on the env var anymore because user could have changed it in the UI already also
@axiomofjoy This is to disable the sweeper (i.e. keeping it dormant) for people who don't realize this has become a new feature and expect to store their spans forever. In other words, the status quo is no sweeping, so we need to keep it that way. |
20fd39e
to
394f665
Compare
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
commit: |
resolves #6611
resolves #6594