-
Notifications
You must be signed in to change notification settings - Fork 712
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
Link scope-ui graphs clickable to prometheus queries #2664
Conversation
prog/main.go
Outdated
@@ -353,6 +354,7 @@ func setupFlags(flags *flags) { | |||
flag.IntVar(&flags.app.memcachedCompressionLevel, "app.memcached.compression", gzip.DefaultCompression, "How much to compress reports stored in memcached.") | |||
flag.StringVar(&flags.app.userIDHeader, "app.userid.header", "", "HTTP header to use as userid") | |||
flag.BoolVar(&flags.app.externalUI, "app.externalUI", false, "Point to externally hosted static UI assets") | |||
flag.StringVar(&flags.app.metricsGraphURL, "app.metrics-graph", "", "Enable extended metrics graph by providing a templated URL (supports :orgID and :query)") |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
render/detailed/links.go
Outdated
} | ||
params := &queryParams{[]cell{{[]string{query}}}} | ||
|
||
bs, err := json.Marshal(params) |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
Please fix the linting errors. |
I note that #2620 anticipates some further additions, e.g. StatefulSet/PetSet. Should we also anticipate them here? |
render/detailed/links.go
Outdated
// Prometheus queries for topologies | ||
topologyQueries = map[string]map[string]*template.Template{ | ||
report.Pod: { | ||
docker.MemoryUsage: prepareTemplate(`sum(container_memory_usage_bytes{pod_name="{{.Label}}"})`), |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
render/detailed/links.go
Outdated
"github.com/ugorji/go/codec" | ||
) | ||
|
||
// MetricLink describes a link referencing a metric. |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
render/detailed/links.go
Outdated
|
||
var ( | ||
// As configured by the user | ||
metricsGraphURL = "" |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
9c3e7c9
to
b43dfd2
Compare
388a599
to
9c57959
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, only minor issues.
UX has been discussed separately. This is a good first step.
|
||
onClick() { | ||
trackMixpanelEvent('scope.node.metric.click', { | ||
layout: GRAPH_VIEW_MODE, |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
@@ -20,7 +21,12 @@ export default class NodeDetailsHealth extends React.Component { | |||
} | |||
|
|||
render() { | |||
const metrics = this.props.metrics || []; | |||
const { | |||
metrics = makeList(), |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
|
||
onClick() { | ||
trackMixpanelEvent('scope.node.metric.click', { | ||
layout: TABLE_VIEW_MODE, |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
}); | ||
} | ||
|
||
static dismissEvent(ev) { |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
@@ -109,7 +109,7 @@ function getSortedNodes(nodes, sortedByHeader, sortedDesc) { | |||
const getValue = getValueForSortedBy(sortedByHeader); | |||
const withAndWithoutValues = groupBy(nodes, (n) => { | |||
const v = getValue(n); | |||
return v !== null && v !== undefined ? 'withValues' : 'withoutValues'; | |||
return !n.valueEmpty && v !== null && v !== undefined ? 'withValues' : 'withoutValues'; |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
|
||
export function darkenColor(c) { | ||
let color = hsl(c); | ||
if (hsl.l < 0.5) { |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
client/package.json
Outdated
"d3-zoom": "1.1.4", | ||
"dagre": "0.7.4", | ||
"debug": "2.6.6", | ||
"filesize": "3.5.9", | ||
"filter-invalid-dom-props": "^2.0.0", |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
<NodeDetailsHealth | ||
metrics={details.metrics} | ||
topologyId={topologyId} | ||
nodeColor={nodeColor} |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
render/detailed/links.go
Outdated
} | ||
|
||
bs.Reset() | ||
if err := tpl.Execute(&bs, summary); err != nil { |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
return "" | ||
} | ||
|
||
if metricsGraphURL[len(metricsGraphURL)-1] != '/' { |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
f1be917
to
21d909d
Compare
@@ -12,6 +12,10 @@ | |||
// TODO: Remove this line once Service UI CONFIGURE button stops being added to Scope. | |||
.scope-wrapper .setup-nav-button { display: none; } | |||
|
|||
a { | |||
text-decoration: none; |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
6931aa6
to
69c4430
Compare
scope-app: - Adds `-app.metrics-graph` cli flag for configuring the base url to use for graph links; supports `:orgID` and `:query` placeholders - Renders `metric_links` in node detail API response scope-ui: - Extends `<CloudFeature />` with option `alwaysShow` and adds boolean `isCloud` property - Links metric graphs in the ui's node details view for all k8s toplogies; or displays placeholder graph if no metrics available
The initial idea was to keep it separate since the unattached links were also to be displayed distinctively from the metrics. With the new design, unattached links are rendered in the same list as metrics with attached links. Therefore, we treat unattached metric links as an empty metric.
This reverts commit 1a00288.
And more DRY.
This reverts commit 4e73801.
69c4430
to
b47cfbd
Compare
Latest JS changes LGTM. Cant wait to see it on dev! |
render/detailed/connections.go
Outdated
output := []Connection{} | ||
for row, count := range c.counts { | ||
// Use MakeNodeSummary to render the id and label of this node | ||
// TODO(paulbellamy): Would be cleaner if we hade just a | ||
// MakeNodeID(ns[row.remoteNodeID]). As we don't need the whole summary. | ||
summary, _ := MakeNodeSummary(r, ns[row.remoteNodeID]) | ||
summary, _ := MakeNodeSummary(r, ns[row.remoteNodeID], metricsGraphURL) |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
render/detailed/links.go
Outdated
@@ -15,7 +15,7 @@ import ( | |||
// MetricLink describes a URL referencing a metric. | |||
type MetricLink struct { | |||
// References the metric id | |||
ID string `json:"id,omitempty"` | |||
ID string `json:"id"` |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
scope-app:
-app.metrics-graph
cli flag for configuring the base url touse for graph links; supports
:orgID
and:query
placeholdersscope-ui:
<CloudFeature />
with optionalwaysShow
<CloudLink />
to simplify routing when in cloud vs not in cloudtoplogies and containers so far
TODO
integration tests(postponed)As long as there is no
--app.metrics-graph
configured, this features is invisible.Screenshots