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

Make Tide record action history (in memory for now) and display via Deck. #10530

Merged
merged 3 commits into from Jan 8, 2019

Conversation

@cjwagner
Copy link
Member

cjwagner commented Dec 21, 2018

I'm still working on the actual front end, but the the backend and plumbing is done.
Design Doc: https://docs.google.com/document/d/1MAphWpgS4vEEGZxkyy8ab-qNU3LAyttRaVcfBI0iY7k/edit

Optional persistent storage of action history in GCS will be added in a follow up PR. The history will be loaded when Tide starts and saved both periodically and when Tide exits.

/kind feature
/cc @ibzib @stevekuznetsov

@ibzib
Copy link
Collaborator

ibzib left a comment

mostly lgtm, picked some nits fwiw


// slice is the cached, in-order slice. Use toSlice(), don't access directly.
// We cache this value because most pools don't change between sync loops.
slice []*Record

This comment has been minimized.

@ibzib

ibzib Dec 21, 2018

Collaborator

nit: slice and q are not very descriptive names considering they share the same type

This comment has been minimized.

@BenTheElder

BenTheElder Dec 22, 2018

Member

yeah, these are pretty bad. maybe buff and cachedSlice or something?

This comment has been minimized.

@cjwagner

cjwagner Jan 4, 2019

Member

Agreed, fixed.

for pool, records := range hist {
needsHide := matches(strings.Split(pool, ":")[0], ta.hiddenRepos)
if (needsHide && ta.hiddenOnly) ||
(!needsHide && !ta.hiddenOnly) {

This comment has been minimized.

@ibzib

ibzib Dec 21, 2018

Collaborator

if needsHide == ta.hiddenOnly ?

This comment has been minimized.

@cjwagner

cjwagner Jan 4, 2019

Member

Done.

ta.Lock()
defer ta.Unlock()
ta.pools = pools
return nil
}

func (ta *tideAgent) filterHidden(tideQueries []config.TideQuery, pools []tide.Pool) ([]config.TideQuery, []tide.Pool) {
func (ta *tideAgent) updateHistory() error {
path := strings.TrimSuffix(ta.path, "/") + "/history"

This comment has been minimized.

@ibzib

ibzib Dec 21, 2018

Collaborator

consider using path.Join here

This comment has been minimized.

@cjwagner

cjwagner Jan 4, 2019

Member

path.Join doesn't work with URLs because they contain consecutive /s. (e.g. path.Join("https://foo", "/bar") == "https:/foo/bar")

I could join the paths as described here: https://stackoverflow.com/questions/34668012/combine-url-paths-with-path-join, but that can error and seems like overkill given this design. If that option is preferable for some reason though I'm happy to switch to that.

func (h *History) ServeHTTP(w http.ResponseWriter, r *http.Request) {
b, err := json.Marshal(h.AllRecords())
if err != nil {
logrus.WithError(err).Error("Encoding YAML history.")

This comment has been minimized.

@ibzib

ibzib Dec 21, 2018

Collaborator

YAML?

This comment has been minimized.

@cjwagner

cjwagner Jan 4, 2019

Member

Fixed.

// - no-filter. i.e. all actions tide has taken in order.

//
// ??? Should `History` use a single `*recordLog` instead of per pool?

This comment has been minimized.

@ibzib

ibzib Dec 21, 2018

Collaborator

I think using multiple record logs is probably best. We could even make logSizeLimit optionally configurable per pool to give busy pools more history, but that's possibly overkill

@stevekuznetsov
Copy link
Contributor

stevekuznetsov left a comment

Worried we're prematurely optimizing, otherwise this seems OK. FWIW I've always needed more interesting data when debugging tide -- hence all the Debug() logging. This will be useful for some end-users, I guess, but I would expect admins to need the logs still

@@ -62,7 +62,7 @@ func (agg aggregate) Error() string {
// This should never happen, really.
return ""
}
return fmt.Sprintf("[%s]", strings.Join(agg.Strings(), ", "))
return fmt.Sprintf("[%s]", strings.Join(agg.Strings(), "; "))

This comment has been minimized.

@stevekuznetsov

stevekuznetsov Dec 21, 2018

Contributor

I wouldn't suggest, we copy-pasted this from k-k

This comment has been minimized.

@cjwagner

cjwagner Jan 4, 2019

Member

Switched it back.

This comment has been minimized.

@stevekuznetsov

stevekuznetsov Jan 4, 2019

Contributor

I think you missed pushing a commit?

Show resolved Hide resolved prow/tide/history/history.go
Show resolved Hide resolved prow/tide/tide.go Outdated

@cjwagner cjwagner force-pushed the cjwagner:tide-table branch from 05a1ad7 to b919343 Jan 3, 2019

@k8s-ci-robot k8s-ci-robot added size/XXL and removed size/XL labels Jan 3, 2019

@stevekuznetsov

This comment has been minimized.

Copy link
Contributor

stevekuznetsov commented Jan 3, 2019

@cjwagner LMK when ready for a re-review?

@k8s-ci-robot k8s-ci-robot requested a review from Katharine Jan 4, 2019

@cjwagner
Copy link
Member

cjwagner left a comment

I've addressed the review comments and added a commit containing all the front end stuff.
The /tide-history page is heavily based on the main prow page (index.html) and most of the code is either shared or copied and tweaked. There is a lot more room to DRY out Deck's typescript, but that is outside the scope of this PR.
@Katharine could you please review the last commit? I'm still learning about typescript and web dev so I've probably found some non-idiomatic ways to do things.
/cc @Katharine

Screenshot with dummy data:
tide-history

ta.Lock()
defer ta.Unlock()
ta.pools = pools
return nil
}

func (ta *tideAgent) filterHidden(tideQueries []config.TideQuery, pools []tide.Pool) ([]config.TideQuery, []tide.Pool) {
func (ta *tideAgent) updateHistory() error {
path := strings.TrimSuffix(ta.path, "/") + "/history"

This comment has been minimized.

@cjwagner

cjwagner Jan 4, 2019

Member

path.Join doesn't work with URLs because they contain consecutive /s. (e.g. path.Join("https://foo", "/bar") == "https:/foo/bar")

I could join the paths as described here: https://stackoverflow.com/questions/34668012/combine-url-paths-with-path-join, but that can error and seems like overkill given this design. If that option is preferable for some reason though I'm happy to switch to that.

for pool, records := range hist {
needsHide := matches(strings.Split(pool, ":")[0], ta.hiddenRepos)
if (needsHide && ta.hiddenOnly) ||
(!needsHide && !ta.hiddenOnly) {

This comment has been minimized.

@cjwagner

cjwagner Jan 4, 2019

Member

Done.

@@ -62,7 +62,7 @@ func (agg aggregate) Error() string {
// This should never happen, really.
return ""
}
return fmt.Sprintf("[%s]", strings.Join(agg.Strings(), ", "))
return fmt.Sprintf("[%s]", strings.Join(agg.Strings(), "; "))

This comment has been minimized.

@cjwagner

cjwagner Jan 4, 2019

Member

Switched it back.

func (h *History) ServeHTTP(w http.ResponseWriter, r *http.Request) {
b, err := json.Marshal(h.AllRecords())
if err != nil {
logrus.WithError(err).Error("Encoding YAML history.")

This comment has been minimized.

@cjwagner

cjwagner Jan 4, 2019

Member

Fixed.


// slice is the cached, in-order slice. Use toSlice(), don't access directly.
// We cache this value because most pools don't change between sync loops.
slice []*Record

This comment has been minimized.

@cjwagner

cjwagner Jan 4, 2019

Member

Agreed, fixed.

Show resolved Hide resolved prow/tide/tide.go Outdated

@cjwagner cjwagner changed the title [WIP] Make Tide record action history (in memory for now) and display via Deck. Make Tide record action history (in memory for now) and display via Deck. Jan 4, 2019

@stevekuznetsov
Copy link
Contributor

stevekuznetsov left a comment

Generally looks good but I think you forgot to push a commit?

if !equality.Semantic.DeepEqual(gotQueries, test.expectedQueries) {
t.Errorf("expected queries:\n%v\ngot queries:\n%v\n", test.expectedQueries, gotQueries)
}
if !equality.Semantic.DeepEqual(gotPools, test.expectedPools) {
t.Errorf("expected pools:\n%v\ngot pools:\n%v\n", test.expectedPools, gotPools)
}
// equality.Semantic.DeepEqual doesn't like the unexported fields in time.Time.

This comment has been minimized.

@stevekuznetsov

stevekuznetsov Jan 4, 2019

Contributor

We're not dealing with k8s objects anyway so we don't need semantic equality here

@@ -62,7 +62,7 @@ func (agg aggregate) Error() string {
// This should never happen, really.
return ""
}
return fmt.Sprintf("[%s]", strings.Join(agg.Strings(), ", "))
return fmt.Sprintf("[%s]", strings.Join(agg.Strings(), "; "))

This comment has been minimized.

@stevekuznetsov

stevekuznetsov Jan 4, 2019

Contributor

I think you missed pushing a commit?

@@ -785,6 +810,13 @@ func (c *Controller) mergePRs(sp subpool, prs []PullRequest) error {
SHA: string(pr.HeadRefOID),
MergeMethod: string(mergeMethod),
}); err != nil {
// TODO: Add a config option to abort batches if a PR in the batch

This comment has been minimized.

@stevekuznetsov

stevekuznetsov Jan 4, 2019

Contributor

Maybe better as a GH Issue rather than a TODO?

This comment has been minimized.

@cjwagner

@cjwagner cjwagner force-pushed the cjwagner:tide-table branch from b919343 to d2c5005 Jan 4, 2019

@cjwagner

This comment has been minimized.

Copy link
Member

cjwagner commented Jan 4, 2019

I think you forgot to push a commit?

Yeah, I did 🤦‍♂️
Pushed now.

@Katharine
Copy link
Member

Katharine left a comment

I only reviewed the TS/HTML. Looks generally good, a bunch of mostly-nits.

General comment: instead of assembling strings by concatenation (e.g. "https://github.com/" + repo + "/commit/" + sha), it can be easier to use template literals (i.e. string interpolation):

`https://github.com/${repo}/commit/${sha}`

This is mostly just an observation: there's not much reason to change the code you already wrote and it compiles to roughly the same thing.

import moment from "moment";

// The cell class contains static methods for constructing common table cells.
export class cell {

This comment has been minimized.

@Katharine

Katharine Jan 4, 2019

Member

I'm not a huge fan of using a static class as effectively a namespacing mechanism. I think it'd make more sense to create a namespace for this:

export namespace cell {
    let idCounter =  0;
    export function text(text: string): HTMLTableDataCellElement {
        // ...
    }
    // ...
}

Anything not exported is effectively private in this setup.

This comment has been minimized.

@cjwagner

cjwagner Jan 5, 2019

Member

👍 Fixed.


static idCounter: number = 0;

static Text(text: string): HTMLTableDataCellElement {

This comment has been minimized.

@Katharine

Katharine Jan 4, 2019

Member

Functions and methods should have lowercase first letters.

This comment has been minimized.

@cjwagner

cjwagner Jan 5, 2019

Member

Fixed.

}
}

export class tooltip {

This comment has been minimized.

@Katharine

Katharine Jan 4, 2019

Member

Same comments as for cell here.

This comment has been minimized.

@cjwagner

cjwagner Jan 5, 2019

Member

Done.

// The cell class contains static methods for constructing common table cells.
export class cell {

static idCounter: number = 0;

This comment has been minimized.

@Katharine

Katharine Jan 4, 2019

Member

nit: the type number is inferred and can be omitted.

This comment has been minimized.

@cjwagner

cjwagner Jan 5, 2019

Member

Fixed.

pulls: {},
};

const hist: {[key: string]: Record[]} = typeof tideHistory !== 'undefined' ? tideHistory.History : {};

This comment has been minimized.

@Katharine

Katharine Jan 4, 2019

Member

This check implies that it's possible for tideHistory to not be defined at all. Is that true, and is it reasonable to silently ignore it?

I think this can't actually happen, in which case the check should be removed. If this really can happen, consider changing the type on line 5 to HistoryData | undefined (which still doesn't quite express that the name might just not exist; I don't think you can express that).

This comment has been minimized.

@cjwagner

cjwagner Jan 5, 2019

Member

tideHistory is undefined if the /tide-history.js?var=tideHistory endpoint is unreachable. We should probably expose an error to distinguish this from no data. How would you recommend that I expose the error to the user?

This comment has been minimized.

@Katharine

Katharine Jan 5, 2019

Member

I don't think we handle this anywhere else. There isn't a super elegant way to deal with it, given our somewhat awkward approach to loading in data via <script> tags. We can either punt and add to the todo list for cleaning up data loading everywhere else, or in the interim you could do something like this in the template:

<script type="text/javascript">
var tideHistory = undefined;
</script>
<script type="text/javascript" src="/tide-history.js?var=tideHistory">

Then you can reasonably change the declaration of tideHistory to be of type HistoryData | undefined, and then test for its existence (using just if (tideHistory)) before doing any real work. Then you can report the missing data — perhaps add a hidden-by-default error report element above the table and show it if something's broken. We won't have any sort of useful error message until we clean up the data loading, so it'd have to be something relatively generic.

This comment has been minimized.

@cjwagner

cjwagner Jan 7, 2019

Member

We can either punt and add to the todo list for cleaning up data loading everywhere else, or in the interim you could do something like this in the template

Since this could affect every Prow page, I'll punt for now and we can fix this generically later. I don't this has ever actually been an issue in practice.


const recs: Record[] = hist[poolKey];
for (let i = 0; i < recs.length; i++) {
const rec: Record = recs[i];

This comment has been minimized.

@Katharine

Katharine Jan 4, 2019

Member

nit: this type can be inferred.

This comment has been minimized.

@cjwagner

cjwagner Jan 5, 2019

Member

Removed.

records.appendChild(r);
}
const recCount = document.getElementById("record-count")!;
recCount.textContent = "Showing " + Math.min(totalCount, 500) + "/" + totalCount + " records";

This comment has been minimized.

@Katharine

Katharine Jan 4, 2019

Member

Maybe pull this 500 and the above 499 out into a constant? Or also see the comment about totalCount.

This comment has been minimized.

@cjwagner

cjwagner Jan 5, 2019

Member

Done.

}

let lastKey = '';
let totalCount = 0;

This comment has been minimized.

@Katharine

Katharine Jan 4, 2019

Member

Is totalCount useful? It's equivalent to i + 1 throughout the loop, and is equal to recs.length by the end of it, but you keep looping just to count it up. Maybe something like this would be cleaner:

const displayedItems = Math.min(recs.length, 500);
for (let i = 0; i < displayedItems; i++) {
  // ...
}
recCount.textContent = `Showing ${displayedItems}/${recs.length} records`;

This comment has been minimized.

@cjwagner

cjwagner Jan 5, 2019

Member

Fixed.

const param = getParameterByName(selectID);
for (let i = 0; i < options.length; i++) {
const o = document.createElement("option");
o.text = options[i];

This comment has been minimized.

@Katharine

Katharine Jan 4, 2019

Member

Perhaps add o.value = options[i] here too (see also comment about selectionText).

This comment has been minimized.

@cjwagner

cjwagner Jan 5, 2019

Member

Done.

err: string;

// The following are not initially present and are instead populated based on the 'History' map key while filtering.
repo: string;

This comment has been minimized.

@Katharine

Katharine Jan 4, 2019

Member

(GitHub managed to discard this comment, so here I go again...)

The approach I've taken with these is to make the interfaces in api/ match exactly the contents of the JSON provided by the backend. I'd prefer to keep that approach. Instead, I would create a new interface that extends this one in tide-history/tide-history.ts:

interface FilteredRecord extends Record {
  // The following are not initially present and are instead populated based on the 'History' map key while filtering.
  repo: string;
  branch: string;
}

Then in redraw(), on line 185 you can change filteredRecs to be a FilteredRecord[], and construct it using new records by changing the construction of rec on line 200 to just this (and removing the following assignments):

const rec: FilteredRecord = {repo, branch, ...recs[i]};

You'll also have to change some function signatures (by my count, that's redrawRecords and targetCell) and remove some Record or Record[] type annotations (which I flagged as nits anyway because they are unnecessary).


If you disagree with my above suggestion, you should at least mark these as optional:

repo?: string
branch?: string

If you do this you'll have some errors to correct elsewhere about them being potentially-undefined.

This comment has been minimized.

@cjwagner

cjwagner Jan 5, 2019

Member

I switched to the pattern you recommended. 👍

Related question: some of the fields are annotated as omitempty fields in the go code. Should the corresponding fields in the TS api files use ? or will the fields receive the empty value if the JSON payload doesn't specify the field.

This comment has been minimized.

@Katharine

Katharine Jan 5, 2019

Member

The corresponding fields should use ?; they'll be omitted if the JSON payload doesn't specify the field.

Effectively the declare const tideHistory: TideHistory is an implicit unsafe cast; if it turns out to not conform to the promised type you will find out when you try using the missing properties at runtime.

This comment has been minimized.

@cjwagner

cjwagner Jan 7, 2019

Member

Ok, fixed.

@cjwagner cjwagner force-pushed the cjwagner:tide-table branch from d2c5005 to d512084 Jan 4, 2019

@stevekuznetsov

This comment has been minimized.

Copy link
Contributor

stevekuznetsov commented Jan 4, 2019

Backend
/lgtm

/hold
For front end

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 4, 2019

LGTM label has been added.

Git tree hash: e6d1386ea4a51fd732bcb6f00c6d4454beee6a71

@cjwagner

This comment has been minimized.

Copy link
Member

cjwagner commented Jan 5, 2019

@Katharine Thanks for the review!
I've addressed most of your comments, but have a couple questions remaining. PTAL

@cjwagner cjwagner force-pushed the cjwagner:tide-table branch from 7e5ef0a to 3dc5541 Jan 7, 2019

@cjwagner

This comment has been minimized.

Copy link
Member

cjwagner commented Jan 7, 2019

@Katharine I think everything has been addressed now. PTAL.

@Katharine
Copy link
Member

Katharine left a comment

Looks good to me!

/lgtm

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 7, 2019

LGTM label has been added.

Git tree hash: 7b1e428a9b78af80baf55435621395d0ee7348e8

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 7, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjwagner, Katharine

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@cjwagner

This comment has been minimized.

Copy link
Member

cjwagner commented Jan 8, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot merged commit fd1ab85 into kubernetes:master Jan 8, 2019

12 checks passed

cla/linuxfoundation cjwagner authorized
Details
pull-test-infra-bazel Job succeeded.
Details
pull-test-infra-gubernator Skipped
pull-test-infra-lint Job succeeded.
Details
pull-test-infra-verify-bazel Job succeeded.
Details
pull-test-infra-verify-codegen Job succeeded.
Details
pull-test-infra-verify-config Job succeeded.
Details
pull-test-infra-verify-deps Skipped
pull-test-infra-verify-gofmt Job succeeded.
Details
pull-test-infra-verify-govet Job succeeded.
Details
pull-test-infra-verify-labels Skipped
tide In merge pool.
Details

@cjwagner cjwagner referenced this pull request Jan 8, 2019

Open

Tracking issue: Tide action history page. #10646

0 of 6 tasks complete
@cjwagner

This comment has been minimized.

Copy link
Member

cjwagner commented Jan 8, 2019

FYI, nothing will actually link to this dashboard yet and I haven't advertised it to anyone. I opened a tracking issue for follow up steps once we see that the page works in prod: #10646

@cjwagner cjwagner deleted the cjwagner:tide-table branch Jan 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment