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.

Sign up for GitHub

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

Let user know which tags are missing per run #967

Merged
merged 2 commits into from
Feb 13, 2018
Merged

Let user know which tags are missing per run #967

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
002af19
Let user know which tags are missing per run
chihuahua Feb 10, 2018
53e116d
Only show warning if missing 1 or 2 tags
chihuahua Feb 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 99 additions & 27 deletions .../plugins/custom_scalar/tf_custom_scalar_dashboard/tf-custom-scalar-margin-chart-card.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,31 +99,47 @@ <h1>[[_titleDisplayString]]</h1>
</template>
</div>

<template is="dom-if" if="[[_tagsWithNoData]]">
<div id="error-content">
<iron-icon class="error-icon" icon="icons:error"></iron-icon>
No data found for these tags:
<ul>
<template is="dom-repeat" items="[[_tagsWithNoData]]">
<li>[[item]]</li>
</template>
</ul>
<br>
Tags for the value, lower, and upper bounds of margin charts in the Layout
proto must match tags in the SCALARS dashboard.
<!-- here -->
<template is="dom-if" if="[[_enumeratedMissingTags.length]]">
<div class="collapsible-list-title">
<paper-icon-button
icon="[[_getToggleCollapsibleIcon(_missingTagsCollapsibleOpened)]]"
on-click="_toggleMissingTagsCollapsibleOpen"
class="toggle-collapsible-button">
</paper-icon-button>
<span class="collapsible-title-text">
<iron-icon icon="icons:error"></iron-icon> Missing Tags
</span>
</div>
<iron-collapse opened="[[_missingTagsCollapsibleOpened]]">
<div class="error-content">
<iron-icon class="error-icon" icon="icons:error"></iron-icon>
<template is="dom-repeat"
items="[[_enumeratedMissingTags]]"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than having _enumeratedMissingTags defined as a separate property (which was confusing me since it seemed like basically the same data as the tagsToMissingRuns property), maybe we can just locally convert it here with a generic helper? Like this: https://stackoverflow.com/a/30794220/1179226

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored per other comment.

as="missingEntry">
<div class="missing-tags-for-run-container">
Run "[[missingEntry.run]]" lacks data for tags
<ul>
<template is="dom-repeat" items="[[missingEntry.tags]]" as="tag">
<li>[[tag]]</li>
</template>
</ul>
</div>
</template>
</div>
</iron-collapse>
</template>

<template is="dom-if" if="[[_tagFilterInvalid]]">
<div id="error-content">
<div class="error-content">
<iron-icon class="error-icon" icon="icons:error"></iron-icon>
This regular expresion is invalid:<br>
<span class="invalid-regex">[[_tagFilter]]</span>
</div>
</template>

<template is="dom-if" if="[[_stepsMismatch]]">
<div id="error-content">
<div class="error-content">
<iron-icon class="error-icon" icon="icons:error"></iron-icon>
The steps for value, lower, and upper tags do not match:
<ul>
Expand All @@ -144,16 +160,16 @@ <h1>[[_titleDisplayString]]</h1>
</template>

<div id="matches-container">
<div id="matches-list-title">
<div class="collapsible-list-title">
<template is="dom-if" if="[[_dataSeriesStrings.length]]">
<paper-icon-button
icon="[[_getToggleMatchesIcon(_matchesListOpened)]]"
icon="[[_getToggleCollapsibleIcon(_matchesListOpened)]]"
on-click="_toggleMatchesOpen"
class="toggle-matches-button">
</paper-icon-button>
</template>

<span class="matches-text">
<span class="collapsible-title-text">
Matches ([[_dataSeriesStrings.length]])
</span>
</div>
Expand All @@ -179,7 +195,7 @@ <h1>[[_titleDisplayString]]</h1>
</div>
<style include="tf-custom-scalar-card-style"></style>
<style>
#error-content {
.error-content {
background: #f00;
border-radius: 5px;
color: #fff;
Expand All @@ -197,7 +213,7 @@ <h1>[[_titleDisplayString]]</h1>
font-weight: bold;
}

#error-content ul {
.error-content ul {
margin: 1px 0 0 0;
padding: 0 0 0 19px;
}
Expand All @@ -206,10 +222,14 @@ <h1>[[_titleDisplayString]]</h1>
font-weight: bold;
}

#matches-list-title {
.collapsible-list-title {
margin: 10px 0 5px 0;
}

.collapsible-title-text {
vertical-align: middle;
}

#matches-list {
font-size: 0.8em;
max-height: 200px;
Expand All @@ -226,8 +246,8 @@ <h1>[[_titleDisplayString]]</h1>
width: 10px;
}

.matches-text {
vertical-align: middle;
.missing-tags-for-run-container {
margin: 8px 0 0 0;
}
</style>
</template>
Expand Down Expand Up @@ -381,7 +401,29 @@ <h1>[[_titleDisplayString]]</h1>
];
}
},
_tagsWithNoData: String,
/**
* This object maps a run to a list of tags (of entries of scalar
* values) that are missing for that run. A single run may be missing up
* to 3 scalar series (for value, lower, and upper).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, wait so does this mean that we show a "missing tags" error for any run at all that doesn't have all three tags? That definitely seems like too noisy an error - it seems like it would appear for almost any typical usage of the margin charts functionality unless every scalar tag is part of a margin plot.

My understanding was that these warnings were to tell you only when there's a case where a run has some of the necessary tags but not all of them. So it would make sense to me to only say a run is "missing" tags if it has at least 1 of the three defined tags. In that sense, it could be missing at most 2 other tags.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, we now only show warnings if a run is missing some but not all tags.

*/
_tagsToMissingRuns: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't runsToMissingTags make more sense as a name? Since it's runs mapped to tags, not tags mapped to runs, and the tags rather than the runs which are missing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah thanks! Did away with this property.

type: Object,
value: {},
},
/**
* A list of objects encapsulating missing tags. Each object within this
* list has the following properties:
* run: A string denoting the relevant run.
* tags: A non-empty list of tags (strings) missing for that run.
*/
_enumeratedMissingTags: {
type: Array,
computed: '_computeEnumeratedMissingTags(_tagsToMissingRuns)',
},
_missingTagsCollapsibleOpened: {
type: Boolean,
value: false,
},
/**
* This field is only set if data retrieved from the server exhibits a
* step mismatch: if the lists of values, lower bounds, and upper bounds
Expand Down Expand Up @@ -518,9 +560,26 @@ <h1>[[_titleDisplayString]]</h1>
}
});

const priorListOfTags = this._tagsToMissingRuns[run];
if (tagsNotFound.length) {
// At least 1 tag could not be found. Show an error message.
this.set('_tagsWithNoData', tagsNotFound);
// At least 1 tag could not be found. Show a warning message.
if (!_.isEqual(priorListOfTags, tagsNotFound)) {
this._tagsToMissingRuns[run] = tagsNotFound;
// We trigger notifications in polymer by creating a new object.
// Polymer's property observers do not work for properties with
// names containing periods, and run names may have periods.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it's a bit unfortunate to not be able to use the native mutation notification logic. I wonder if we could work around this by instead storing enumeratedMissingTags directly (so in contravention of my comment above) and then doing the mutations on that using the built-in array mutation methods.

The only caveat would be not being able to look up a run's missing tag entry directly by the run name (one would instead have to iterate through the array). But we could fix that by just keeping a separate runToMissingTagsIndex property that maps run names to indexes in the array. (In fact, I think we could even technically use the same property object by just adding the runs as properties on the array object, though maybe that's a little too hacky.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using array mutations works!

this.set(
'_tagsToMissingRuns',
Object.assign({}, this._tagsToMissingRuns));
}
} else {
if (priorListOfTags) {
// Tags that had previously been missing are now found.
delete this._tagsToMissingRuns[run];
this.set(
'_tagsToMissingRuns',
Object.assign({}, this._tagsToMissingRuns));
}
}

this.set('_nameToDataSeries', newMapping);
Expand Down Expand Up @@ -606,8 +665,8 @@ <h1>[[_titleDisplayString]]</h1>
_escapeRegexCharacters(stringValue) {
return stringValue.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
},
_getToggleMatchesIcon(matchesListOpened) {
return matchesListOpened ? 'expand-less' : 'expand-more';
_getToggleCollapsibleIcon(listOpened) {
return listOpened ? 'expand-less' : 'expand-more';
},
_toggleMatchesOpen() {
this.set('_matchesListOpened', !this._matchesListOpened);
Expand All @@ -620,6 +679,19 @@ <h1>[[_titleDisplayString]]</h1>
_separateWithCommas(numbers) {
return numbers.join(', ');
},
_computeEnumeratedMissingTags(tagsToMissingRuns) {
return _.map(tagsToMissingRuns, (tags, run) => {
return {
run: run,
tags: tags,
};
})
},
_toggleMissingTagsCollapsibleOpen() {
this.set(
'_missingTagsCollapsibleOpened',
!this._missingTagsCollapsibleOpened);
},
});
</script>
</dom-module>