Commit 7a7a5467 authored by Sean McGivern's avatar Sean McGivern

Show warnings in performance bar

Right now, the performance bar is unopinionated: it will tell you how
many SQL queries a page ran, but not if that's good or bad. It also
hides away AJAX requests behind a dropdown, which means people are less
likely to see those requests.

This adds a way of highlighting warnings generated by the backend in
both places. In a request itself, a metric will have a warning item if
the backend thinks it's too slow. And in the request selector, we
highlight requests that have warnings associated.
parent 23d5fc34
<script> <script>
import requestWarning from './request_warning.vue';
import DeprecatedModal2 from '~/vue_shared/components/deprecated_modal_2.vue'; import DeprecatedModal2 from '~/vue_shared/components/deprecated_modal_2.vue';
import Icon from '~/vue_shared/components/icon.vue'; import Icon from '~/vue_shared/components/icon.vue';
export default { export default {
components: { components: {
requestWarning,
GlModal: DeprecatedModal2, GlModal: DeprecatedModal2,
Icon, Icon,
}, },
...@@ -39,6 +42,13 @@ export default { ...@@ -39,6 +42,13 @@ export default {
detailsList() { detailsList() {
return this.metricDetails.details; return this.metricDetails.details;
}, },
htmlId() {
if (this.currentRequest) {
return `performance-bar-warning-${this.currentRequest.id}-${this.metric}`;
}
return '';
},
}, },
}; };
</script> </script>
...@@ -105,5 +115,6 @@ export default { ...@@ -105,5 +115,6 @@ export default {
<div slot="footer"></div> <div slot="footer"></div>
</gl-modal> </gl-modal>
{{ title }} {{ title }}
<request-warning :html-id="htmlId" :details="metricDetails" />
</div> </div>
</template> </template>
<script> <script>
import { glEmojiTag } from '~/emoji';
export default { export default {
props: { props: {
currentRequest: { currentRequest: {
...@@ -15,6 +17,11 @@ export default { ...@@ -15,6 +17,11 @@ export default {
currentRequestId: this.currentRequest.id, currentRequestId: this.currentRequest.id,
}; };
}, },
computed: {
anyRequestsWithWarnings() {
return this.requests.some(request => request.hasWarnings);
},
},
watch: { watch: {
currentRequestId(newRequestId) { currentRequestId(newRequestId) {
this.$emit('change-current-request', newRequestId); this.$emit('change-current-request', newRequestId);
...@@ -31,6 +38,7 @@ export default { ...@@ -31,6 +38,7 @@ export default {
return truncated; return truncated;
}, },
glEmojiTag,
}, },
}; };
</script> </script>
...@@ -44,7 +52,9 @@ export default { ...@@ -44,7 +52,9 @@ export default {
class="qa-performance-bar-request" class="qa-performance-bar-request"
> >
{{ truncatedUrl(request.url) }} {{ truncatedUrl(request.url) }}
<span v-if="request.hasWarnings" v-html="glEmojiTag('warning')"></span>
</option> </option>
</select> </select>
<span v-if="anyRequestsWithWarnings" v-html="glEmojiTag('warning')"></span>
</div> </div>
</template> </template>
<script>
import { glEmojiTag } from '~/emoji';
import { GlPopover } from '@gitlab/ui';
export default {
components: {
GlPopover,
},
props: {
htmlId: {
type: String,
required: true,
},
details: {
type: Object,
required: true,
},
},
computed: {
warnings() {
const {
details: { warnings },
} = this;
return warnings && warnings.length ? warnings : null;
},
warningMessage() {
if (!this.warnings) {
return '';
}
return this.warnings.join('\n');
},
},
methods: {
glEmojiTag,
},
};
</script>
<template>
<span v-if="warnings">
<span :id="htmlId" v-html="glEmojiTag('warning')"></span>
<gl-popover :target="htmlId" :content="warningMessage" triggers="hover focus" />
</span>
</template>
...@@ -41,7 +41,7 @@ export default ({ container }) => ...@@ -41,7 +41,7 @@ export default ({ container }) =>
PerformanceBarService.fetchRequestDetails(this.peekUrl, requestId) PerformanceBarService.fetchRequestDetails(this.peekUrl, requestId)
.then(res => { .then(res => {
this.store.addRequestDetails(requestId, res.data.data); this.store.addRequestDetails(requestId, res.data);
}) })
.catch(() => .catch(() =>
// eslint-disable-next-line no-console // eslint-disable-next-line no-console
......
...@@ -3,12 +3,13 @@ export default class PerformanceBarStore { ...@@ -3,12 +3,13 @@ export default class PerformanceBarStore {
this.requests = []; this.requests = [];
} }
addRequest(requestId, requestUrl, requestDetails) { addRequest(requestId, requestUrl) {
if (!this.findRequest(requestId)) { if (!this.findRequest(requestId)) {
this.requests.push({ this.requests.push({
id: requestId, id: requestId,
url: requestUrl, url: requestUrl,
details: requestDetails, details: {},
hasWarnings: false,
}); });
} }
...@@ -22,7 +23,8 @@ export default class PerformanceBarStore { ...@@ -22,7 +23,8 @@ export default class PerformanceBarStore {
addRequestDetails(requestId, requestDetails) { addRequestDetails(requestId, requestDetails) {
const request = this.findRequest(requestId); const request = this.findRequest(requestId);
request.details = requestDetails; request.details = requestDetails.data;
request.hasWarnings = requestDetails.has_warnings;
return request; return request;
} }
......
---
title: Add warnings to performance bar when page shows signs of poor performance
merge_request: 17612
author:
type: changed
...@@ -39,6 +39,7 @@ describe('detailedMetric', () => { ...@@ -39,6 +39,7 @@ describe('detailedMetric', () => {
duration: '123ms', duration: '123ms',
calls: '456', calls: '456',
details: requestDetails, details: requestDetails,
warnings: ['gitaly calls: 456 over 30'],
}, },
}, },
}, },
...@@ -82,6 +83,10 @@ describe('detailedMetric', () => { ...@@ -82,6 +83,10 @@ describe('detailedMetric', () => {
expect(vm.$el.innerText).toContain('gitaly'); expect(vm.$el.innerText).toContain('gitaly');
}); });
it('displays request warnings', () => {
expect(vm.$el.querySelector('gl-emoji').dataset.name).toEqual('warning');
});
describe('when using a custom metric title', () => { describe('when using a custom metric title', () => {
beforeEach(() => { beforeEach(() => {
vm = mountComponent(Vue.extend(detailedMetric), { vm = mountComponent(Vue.extend(detailedMetric), {
......
...@@ -4,14 +4,25 @@ import mountComponent from 'spec/helpers/vue_mount_component_helper'; ...@@ -4,14 +4,25 @@ import mountComponent from 'spec/helpers/vue_mount_component_helper';
describe('request selector', () => { describe('request selector', () => {
const requests = [ const requests = [
{ id: '123', url: 'https://gitlab.com/' }, {
id: '123',
url: 'https://gitlab.com/',
hasWarnings: false,
},
{ {
id: '456', id: '456',
url: 'https://gitlab.com/gitlab-org/gitlab-foss/merge_requests/1', url: 'https://gitlab.com/gitlab-org/gitlab-foss/merge_requests/1',
hasWarnings: false,
}, },
{ {
id: '789', id: '789',
url: 'https://gitlab.com/gitlab-org/gitlab-foss/merge_requests/1.json?serializer=widget', url: 'https://gitlab.com/gitlab-org/gitlab-foss/merge_requests/1.json?serializer=widget',
hasWarnings: false,
},
{
id: 'abc',
url: 'https://gitlab.com/gitlab-org/gitlab-foss/merge_requests/1/discussions.json',
hasWarnings: true,
}, },
]; ];
...@@ -43,4 +54,15 @@ describe('request selector', () => { ...@@ -43,4 +54,15 @@ describe('request selector', () => {
it('ignores trailing slashes', () => { it('ignores trailing slashes', () => {
expect(optionText(requests[0].id)).toEqual('gitlab.com'); expect(optionText(requests[0].id)).toEqual('gitlab.com');
}); });
it('has a warning icon if any requests have warnings', () => {
expect(vm.$el.querySelector('span > gl-emoji').dataset.name).toEqual('warning');
});
it('adds a warning icon to requests with warnings', () => {
const option = vm.$el.querySelector('[value="abc"]');
expect(option.querySelector('gl-emoji').dataset.name).toEqual('warning');
expect(option.innerText).toContain('discussions.json');
});
}); });
import Vue from 'vue';
import requestWarning from '~/performance_bar/components/request_warning.vue';
import mountComponent from 'spec/helpers/vue_mount_component_helper';
describe('request warning', () => {
const htmlId = 'request-123';
let vm;
afterEach(() => {
vm.$destroy();
});
describe('when the request has warnings', () => {
beforeEach(() => {
vm = mountComponent(Vue.extend(requestWarning), {
htmlId,
details: {
warnings: ['gitaly calls: 30 over 10', 'gitaly duration: 1500 over 1000'],
},
});
});
it('adds a warning emoji with the correct ID', () => {
const wrapper = vm.$el.querySelector('span[id]');
expect(wrapper.id).toEqual(htmlId);
expect(wrapper.querySelector('gl-emoji').dataset.name).toEqual('warning');
});
});
describe('when the request does not have warnings', () => {
beforeEach(() => {
vm = mountComponent(Vue.extend(requestWarning), {
htmlId,
details: {
warnings: [],
},
});
});
it('does nothing', () => {
expect(vm.$el.innerText).toBeUndefined();
});
});
});
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment