Skip to content
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

Merging log and history endpoints #1743

Closed
Arqu opened this issue Apr 6, 2021 · 8 comments
Closed

Merging log and history endpoints #1743

Arqu opened this issue Apr 6, 2021 · 8 comments
Assignees
Labels
API JSON API issues
Milestone

Comments

@Arqu
Copy link
Contributor

Arqu commented Apr 6, 2021

Recently we decided to merge log and history, however that merge currently goes against our HTTP RPC API pattern.
Some quick facts:

  • history returns VersionInfo
  • history also handles network call managed by the source param
  • log returns LogEntry
  • history takes a subset of the LogEntries and then augments them with other info from the dataset to fill in the VersionInfo

In order to bring it where we intended to we need to:

  • Decide what should such a combined endpoint return - LogEntry or VersionInfo
  • How should we handle the network resolution aspect (or just keep as is/branch if it's a ?filter=history request)
  • Alternatively keep them separate?
@Arqu Arqu added the API JSON API issues label Apr 6, 2021
@Arqu Arqu self-assigned this Apr 6, 2021
@ramfox
Copy link
Member

ramfox commented Apr 6, 2021

I can see us having a need for a network call to LogEntries - way to get an activity feed without needing to have the dataset in your logbook/do logsync.

@b5 b5 added this to the v0.10.0 milestone Apr 6, 2021
@b5
Copy link
Member

b5 commented Apr 6, 2021

log returns LogEntry

The only time we need raw log data is when we're debugging. To me this is less a conversation of "merging log into history", as it is keeping history working the way it currently does, and de-prioritizing the log command. History should (still) return a slice of VersionInfo

history also handles network call managed by the source param

This is used by qri desktop, and will also be used in the new UI. We should keep it. @dustmop has guidance on how to handle the source param moving forward, but the TL;DR; remove the Source field from the Param struct, and use inst.WithSource("source").[MethodSet()]

@b5
Copy link
Member

b5 commented Apr 6, 2021

but to address your last question Asmir, I'd agree we should keep them separate. It's clear to me now that we need both /history and /log, but I'd love to rename /log to something more verbose to make it clearer that it's a plumbing command.

/operations?

@ramfox
Copy link
Member

ramfox commented Apr 6, 2021

I disagree that /log is a plumbing command. You can want a list of log entries for a dataset to show failed/no changes runs, or renames, aka a kind of activity log. Maybe this isn't correct looking forward b/c we are merging qrimatic functionality into qri & we will be adding some /ds/activity endpoint?

@Arqu
Copy link
Contributor Author

Arqu commented Apr 7, 2021

I'm leaning towards @ramfox's point of view on this. /log seems like the basis of many upcoming endpoints/functionality. Maybe just it's output format is what drives us to the debugging usage as it's not super useful for consumption.

@b5
Copy link
Member

b5 commented Apr 7, 2021

I disagree that /log is a plumbing command. You can want a list of log entries for a dataset to show failed/no changes runs, or renames, aka a kind of activity log.

We're currently doing this in qrimatic with the /history endpoint. Commit a27e3c6 expanded the definition of a VersionInfo to capture failed runs:

// VersionInfo is an aggregation of fields from a dataset version for caching &
// listing purposes. VersionInfos are typically used when showing a list of
// datasets or a list of dataset versions ("qri list" and "qri log").
//
// VersionInfos can also describe dataset versions that are being created or
// failed to create. In these cases the calculated VersionInfo.Path value must
// always equal the empty string.
//
// If any fields are added to this struct, keep it in sync with:
// dscache/def.fbs dscache
// dscache/fill_info.go func fillInfoForDatasets
// repo/ref/convert.go func ConvertToVersionInfo
// If you are considering making major changes to VersionInfo, read this
// synopsis first:
// https://github.com/qri-io/qri/pull/1641#issuecomment-778521313
type VersionInfo struct {

Since we expanded a few things have changed that are making this look like the story needs to change. #1741 proposes we add workflow scheduling to logbook, which would be nice to have reflected in the primary /history command, but that would require changing the return value of /history to a slice of a heterogeneous type.

Our deign goal should be to have one endpoint that describes the way a dataset has changed over time, and use filters to scope that aggregation down. When @Arqu & @ramfox say "log isn't a plumbing command", I'm hearing: we need to expand the history command to meet our needs.

I still maintain log is a plumbing thing. raw logbook operations in and of themselves aren't useful beyond debugging, first & foremost b/c delete operations need to be applied to produce a history. This will only get more complicated once we introduce merging semantics, where you'll need interpret multiple operation logs to produce a history.

So I think the thing we need to discuss here is changing the return value of history. We can assume the filtering / aggregation parameter conversation will happen on #1737

@ramfox ramfox mentioned this issue Apr 8, 2021
29 tasks
@Arqu
Copy link
Contributor Author

Arqu commented Apr 20, 2021

In #1762 we've cleaned up pieces of this. The raw/summary endpoints have been removed from the API, history is now activity and is the endpoint that should evolve to meet the needs moving forward. log is removed from the API but exists as a cmd operation for debugging purposes. Can be brought back easily.

@Arqu
Copy link
Contributor Author

Arqu commented Apr 26, 2021

Closing since the above is enough for 0.10.0

@Arqu Arqu closed this as completed Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API JSON API issues
Projects
None yet
Development

No branches or pull requests

3 participants