feat(persistence): experimental bulk inserter for spans #2808
Conversation
dosubot
bot
added
the
size:L
RogerHYang
changed the title
| @@ -181,7 +198,16 @@ def create_app( | |||
| debug: bool = False, | |||
| read_only: bool = False, | |||
| enable_prometheus: bool = False, | |||
| initial_spans: Optional[Iterable[Union[Span, Tuple[Span, str]]]] = None, | |||
There was a problem hiding this comment.
| initial_spans: Optional[Iterable[Union[Span, Tuple[Span, str]]]] = None, | |
| initial_spans: Optional[Iterable[Tuple[Span, str]]] = None, |
Suggestion: Simplify the input type. Did we add the union type for handling fixtures? If so, we could just convert the fixtures to use the default project.
There was a problem hiding this comment.
Good point. On the other hand. i have to do that in two places: main.py and session.py. So i opted to just do it in one place (here) instead.
| return self._queue_span | ||
|
|
||
| async def __aexit__(self, *args: Any) -> None: | ||
| self._running = False |
There was a problem hiding this comment.
i'll let the garbage collect do it later. it's harmless either way
| if await session.scalar(select(1).where(models.Span.span_id == span.context.span_id)): | ||
| # Span already exists | ||
| return |
There was a problem hiding this comment.
Is there not a setting to ignore inserts if the record already exists so we don't need to hit the database an extra time?
There was a problem hiding this comment.
Yes, but it'll raise a IntegrityError which is annoying. On the other hand, this operation here is not expensive, because the B-tree is most likely already in the buffer pool.
| # Span already exists | ||
| return | ||
| if not ( | ||
| project_rowid := await session.scalar( |
There was a problem hiding this comment.
Should we start moving away from the rowid naming convention since we are trying to support Postgres in addition to SQLite?
There was a problem hiding this comment.
discussed offline: currently don't have a good alternative name, but will reconsider later
| @@ -0,0 +1,177 @@ | |||
| import asyncio | |||
There was a problem hiding this comment.
nit: the file location of this feels off if it's specific to spans
There was a problem hiding this comment.
it could be used for bulk inserting evals too. it'll just need second queue
resolves #2806
This PR implements the queuing method described below.
Two strategies for write transactions: