fsprojects / FSharp.AWS.DynamoDB

F# wrapper API for AWS DynamoDB
MIT License
58 stars 18 forks source link

Minor tidying #39

Closed bartelink closed 2 years ago

bartelink commented 2 years ago
dsyme commented 2 years ago

@samritchie Checking you're good to be ongoing maintainer on this and can look after this PR, thanks!

bartelink commented 2 years ago

(There's definitely no rush on this, and I might have follow-ups - I'm looking at this in the context of https://github.com/jet/equinox/pull/321 and have a long way to go on that.)

dsyme commented 2 years ago

Cool! Good to see equinox progressing.

@bartelink It might make sense to make you the 2nd maintainer in this repo - we like to have two.

@samritchie what do you think?

dsyme commented 2 years ago

@sergey-tihon We should reduce the number of admins/write/maintain for this repo down to 2, currently there are 5 plus the rwo of us.

sergey-tihon commented 2 years ago

@dsyme I do not think that I can do 2, it was 3. (it is restored)

bartelink commented 2 years ago

Thanks Don; I'm looking forward to seeing if I can map things to make the Equinox patterns fit Dynamo as well as they fit the other supported stores (most notably CosmosDB) to date - there's some dense JS that'll become hopefully legible F# in the process (the Cosmos SDK now has a transactional batch feature that obviates the need to descend into that which was present when the code was first written).

It's great to see that people (esp Sam) have been keeping the issues list here so well-curated. While I don't mind keeping an eye, I can't say I'm well up on either DynamoDB specifics or the specific type juggling fu on which this rests. I have literally not made a Table in a dynamodb-local as yet!

I also don't envisage needing any extensions at the present time that would drive me to dig deeper (while IAsyncEnumerable might help significantly for mainstream usage of Dynamo, being able to provide detailed metrics and/or control of individual roundtrips as I already do in the Cosmos equivalent by explicitly running the underlying queries happens to fit the bill for my usage) - i.e. the nature of my usage of Dynamo as I envisage it now does not involve chopping and changing of indexing strategies or access patterns to any major degree (famous last words of course!)

bartelink commented 2 years ago

I also don't envisage needing any extensions at the present time ... (famous last words of course!)

🤔 the metricsCollector almost works for my context, but I need to be able to separate and/or contextualize metrics per concurrent stream of work. In Equinox, this is achieved by explicitly passing a Serilog ILogger explicitly to any API that can occasion CUs. It seems as it stands that I'd have to spin up (or rent from a pool) a separate TableContext for every concurrent action to be able to contextualize the outputs (that is unless I descend into using ExecutionContext trickery). I may do a PR that adds the ability to supply the collector as an optional arg per method on the TableContext in order to be able to avoid having to spin one up per call (but retaining the existing ability to supply one on a global basis).

samritchie commented 2 years ago

@dsyme I'll have a look at this PR today, I'd also be more than happy to have another maintainer.

@bartelink metricsCollector was a fairly recent (and basic) addition that was driven mostly by my need to be able to measure aggregate per-table consumed capacity without going through Cloudwatch. I'm not wedded to the design and I can see the utility in being able to allocate work to specific traces/requests/actions/whatever. Optional arg per method seems like it should work. I'll do a bit more research around observability in other libraries.

Also despite (or thanks to?) having added the pagination support recently, I feel IAsyncEnumerable would be a better model for scans, and most 'mainstream' paging requirements are better suited to range queries. Switching to a Task interface is something on the todo list, I'll look into it at that stage.

bartelink commented 2 years ago

Also despite (or thanks to?) having added the pagination support recently, I feel IAsyncEnumerable would be a better model for scans, and most 'mainstream' paging requirements are better suited to range queries. Switching to a Task interface is something on the todo list, I'll look into it at that stage.

While I guess dropping to Task might not hurt and is arguably more idiomatic, it does mean you get the joy of passing cancellation tokens. IME (based on Equinox), the Async overhead is not unreasonable in the context of the fact that one is always wrapping a network hop.

Exposing AsyncSeq or IAsyncEnumerable can definitely lead to some client things collapsing very nicely, though being able to have metrics for each of the underlying network hops/follow-on requests can be extremely valuable when analyzing perf/hangs in the context of high load and/or concurrency.

bartelink commented 2 years ago

Regarding ☝️ (i.e., direct provision of IAsyncEnumerable not necessarily ticking all boxes depending on how it's implemented), see EnumBatches in https://github.com/jet/equinox/blob/3d4dc851387e9a076c1f859f965860d1159e2496/src/Equinox.DynamoStore/DynamoStore.fs#L350 for what I'm doing - this enables me to log sub-calls ('slices') of the overall query being performed for detailed diagnostics