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

Add links to make values in tags or log properties clickable #223

Merged
merged 18 commits into from
Aug 3, 2018
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ type AccordianKeyValuesProps = {
highContrast?: boolean,
isOpen: boolean,
label: string,
linksGetter: ?({ key: string, value: any }[], number) => { url: string, text: string }[],
Copy link
Member

Choose a reason for hiding this comment

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

Please use KeyValuePair from src/types/index.js which is the same type. (Just realized it's a mistake in existing code, too... doh!)

type KeyValuePair = {
key: string,
value: any,
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok! Indeed, it is better to use named types. I have updated the code to use this KeyValuePair type and the newly added Link one so that it is more readable.

onToggle: () => void,
};

Expand Down Expand Up @@ -59,7 +60,7 @@ KeyValuesSummary.defaultProps = {
};

export default function AccordianKeyValues(props: AccordianKeyValuesProps) {
const { className, data, highContrast, isOpen, label, onToggle } = props;
const { className, data, highContrast, isOpen, label, linksGetter, onToggle } = props;
const isEmpty = !Array.isArray(data) || !data.length;
const iconCls = cx('u-align-icon', { 'AccordianKeyValues--emptyIcon': isEmpty });
return (
Expand All @@ -80,7 +81,7 @@ export default function AccordianKeyValues(props: AccordianKeyValuesProps) {
</strong>
{!isOpen && <KeyValuesSummary data={data} />}
</div>
{isOpen && <KeyValuesTable data={data} />}
{isOpen && <KeyValuesTable data={data} linksGetter={linksGetter} />}
</div>
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import './AccordianLogs.css';

type AccordianLogsProps = {
isOpen: boolean,
linksGetter: ?({ key: string, value: any }[], number) => { url: string, text: string }[],
logs: Log[],
onItemToggle: Log => void,
onToggle: () => void,
Expand All @@ -35,7 +36,7 @@ type AccordianLogsProps = {
};

export default function AccordianLogs(props: AccordianLogsProps) {
const { isOpen, logs, openedItems, onItemToggle, onToggle, timestamp } = props;
const { isOpen, linksGetter, logs, openedItems, onItemToggle, onToggle, timestamp } = props;

return (
<div className="AccordianLogs">
Expand All @@ -59,6 +60,7 @@ export default function AccordianLogs(props: AccordianLogsProps) {
// compact
highContrast
isOpen={openedItems.has(log)}
linksGetter={linksGetter}
data={log.fields || []}
label={`${formatDuration(log.timestamp - timestamp)}`}
onToggle={() => onItemToggle(log)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,8 @@ limitations under the License.
white-space: pre;
width: 125px;
}

.KeyValueTable--linkIcon {
vertical-align: middle;
font-weight: bold;
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import React from 'react';
import jsonMarkup from 'json-markup';
import { Dropdown, Icon, Menu } from 'antd';
Copy link
Member

Choose a reason for hiding this comment

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

This file looks great! Awesome.

(Not sure how to add a comment to a file instead of a line...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your encouraging feedback!


import './KeyValuesTable.css';

Expand All @@ -30,27 +31,75 @@ function parseIfJson(value) {
return value;
}

function markupAsDiv(markup) {
// eslint-disable-next-line react/no-danger
return <div dangerouslySetInnerHTML={{ __html: markup }} />;
}

function markupAsSpan(markup) {
const spanMarkup = markup.replace(/^<div /i, '<span ').replace(/<\/div>$/i, '</span>');
// eslint-disable-next-line react/no-danger
return <span dangerouslySetInnerHTML={{ __html: spanMarkup }} />;
}
Copy link
Member

Choose a reason for hiding this comment

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

Instead of manipulating the HTML seems like adding ub-inline-block to the <div> would work?

            const jsonTable = (
              // eslint-disable-next-line react/no-danger
              <div className="ub-inline-block" dangerouslySetInnerHTML={{ __html: jsonMarkup(parseIfJson(row.value)) }} />
            );

I think that might also need

.KeyValueTable--body > tr > td {
    padding: 0.25rem 0.5rem;
    vertical-align: top;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your suggestion. I have added a commit with this change.


type KeyValuesTableProps = {
data: { key: string, value: any }[],
linksGetter: ?({ key: string, value: any }[], number) => { url: string, text: string }[],
};

export default function KeyValuesTable(props: KeyValuesTableProps) {
const { data } = props;
const { data, linksGetter } = props;
return (
<div className="KeyValueTable u-simple-scrollbars">
<table className="u-width-100">
<tbody className="KeyValueTable--body">
{data.map((row, i) => {
const jsonTable = (
// eslint-disable-next-line react/no-danger
<div dangerouslySetInnerHTML={{ __html: jsonMarkup(parseIfJson(row.value)) }} />
);
const markup = jsonMarkup(parseIfJson(row.value));
const links = linksGetter ? linksGetter(data, i) : null;
let valueMarkup;
if (links && links.length === 1) {
valueMarkup = (
<div>
<a href={links[0].url} title={links[0].text} target="_blank" rel="noopener noreferrer">
{markupAsSpan(markup)} <Icon className="KeyValueTable--linkIcon" type="export" />
</a>
</div>
);
} else if (links && links.length > 1) {
const menuItems = (
<Menu>
{links.map((link, index) => {
const { text, url } = link;
return (
// `index` is necessary in the key because url can repeat
// eslint-disable-next-line react/no-array-index-key
<Menu.Item key={`${url}-${index}`}>
<a href={url} target="_blank" rel="noopener noreferrer">
{text}
</a>
</Menu.Item>
);
})}
</Menu>
);
valueMarkup = (
<div>
<Dropdown overlay={menuItems} placement="bottomRight" trigger={['click']}>
<a>
{markupAsSpan(markup)} <Icon className="KeyValueTable--linkIcon" type="profile" />
</a>
</Dropdown>
</div>
);
} else {
valueMarkup = markupAsDiv(markup);
}
return (
// `i` is necessary in the key because row.key can repeat
// eslint-disable-next-line react/no-array-index-key
<tr key={`${row.key}-${i}`}>
<td className="KeyValueTable--keyColumn">{row.key}</td>
<td>{jsonTable}</td>
<td>{valueMarkup}</td>
Copy link
Member

Choose a reason for hiding this comment

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

Seems like the code above is sufficiently intricate to warrant being broken down into sub-components at the top of the file, similar to CustomNavDropdown. Something like LinkValue and LinkValueList where LinkValueList would compose LinkValue?

// Individual link
<LinkValue
  href={href}
  title={title}
  content={jsonMarkup}
/>

// List of links
<LinkValueList
  links={links}
  content={jsonMarkup}
/>

What do you think?

Also, I think it would make sense for the link to always show the icon?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have taken into account this suggestion and updated the PR.
Note that the Dropdown component in its overlay property seems to work differently if there is a wrapper component such as LinkValueList before the Menu component, so I don't call LinkValueList as a component (<LinkValueList ... />) but directly as a function ({linkValueList(...)}).

Copy link
Member

Choose a reason for hiding this comment

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

Looks great, thank you.

</tr>
);
})}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

import React from 'react';
import { shallow } from 'enzyme';
import { Dropdown, Icon } from 'antd';

import KeyValuesTable from './KeyValuesTable';

Expand All @@ -38,4 +39,60 @@ describe('<KeyValuesTable>', () => {
expect(tr.find('.KeyValueTable--keyColumn').text()).toMatch(data[i].key);
});
});

it('renders a single link correctly', () => {
wrapper.setProps({
linksGetter: (array, i) =>
array[i].key === 'span.kind'
? [
{
url: `http://example.com/?kind=${encodeURIComponent(array[i].value)}`,
text: `More info about ${array[i].value}`,
},
]
: [],
});

const anchor = wrapper.find('a');
expect(anchor).toHaveLength(1);
expect(anchor.prop('href')).toBe('http://example.com/?kind=client');
expect(anchor.prop('title')).toBe('More info about client');
expect(anchor.find(Icon)).toHaveLength(1);
expect(
anchor
.closest('tr')
.find('td')
.first()
.text()
).toBe('span.kind');
});

it('renders multiple links correctly', () => {
wrapper.setProps({
linksGetter: (array, i) =>
array[i].key === 'span.kind'
? [
{ url: `http://example.com/1?kind=${encodeURIComponent(array[i].value)}`, text: 'Example 1' },
{ url: `http://example.com/2?kind=${encodeURIComponent(array[i].value)}`, text: 'Example 2' },
]
: [],
});
const dropdown = wrapper.find(Dropdown);
const menu = shallow(dropdown.prop('overlay'));
const anchors = menu.find('a');
expect(anchors).toHaveLength(2);
const firstAnchor = anchors.first();
expect(firstAnchor.prop('href')).toBe('http://example.com/1?kind=client');
expect(firstAnchor.text()).toBe('Example 1');
const secondAnchor = anchors.last();
expect(secondAnchor.prop('href')).toBe('http://example.com/2?kind=client');
expect(secondAnchor.text()).toBe('Example 2');
expect(
dropdown
.closest('tr')
.find('td')
.first()
.text()
).toBe('span.kind');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import './index.css';

type SpanDetailProps = {
detailState: DetailState,
linksGetter: ?({ key: string, value: any }[], number) => { url: string, text: string }[],
logItemToggle: (string, Log) => void,
logsToggle: string => void,
processToggle: string => void,
Expand All @@ -37,7 +38,16 @@ type SpanDetailProps = {
};

export default function SpanDetail(props: SpanDetailProps) {
const { detailState, logItemToggle, logsToggle, processToggle, span, tagsToggle, traceStartTime } = props;
const {
detailState,
linksGetter,
logItemToggle,
logsToggle,
processToggle,
span,
tagsToggle,
traceStartTime,
} = props;
const { isTagsOpen, isProcessOpen, logs: logsState } = detailState;
const { operationName, process, duration, relativeStartTime, spanID, logs, tags } = span;
const overviewItems = [
Expand Down Expand Up @@ -73,6 +83,7 @@ export default function SpanDetail(props: SpanDetailProps) {
<AccordianKeyValues
data={tags}
label="Tags"
linksGetter={linksGetter}
isOpen={isTagsOpen}
onToggle={() => tagsToggle(spanID)}
/>
Expand All @@ -81,6 +92,7 @@ export default function SpanDetail(props: SpanDetailProps) {
className="ub-mb1"
data={process.tags}
label="Process"
linksGetter={linksGetter}
isOpen={isProcessOpen}
onToggle={() => processToggle(spanID)}
/>
Expand All @@ -89,6 +101,7 @@ export default function SpanDetail(props: SpanDetailProps) {
{logs &&
logs.length > 0 && (
<AccordianLogs
linksGetter={linksGetter}
logs={logs}
isOpen={logsState.isOpen}
openedItems={logsState.openedItems}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,12 @@ type SpanDetailRowProps = {
detailState: DetailState,
onDetailToggled: string => void,
isFilteredOut: boolean,
linksGetter: ?(number, { key: string, value: any }[], number) => { url: string, text: string }[],
logItemToggle: (string, Log) => void,
logsToggle: string => void,
processToggle: string => void,
span: Span,
spanIndex: number,
tagsToggle: string => void,
traceStartTime: number,
};
Expand All @@ -45,12 +47,18 @@ export default class SpanDetailRow extends React.PureComponent<SpanDetailRowProp
this.props.onDetailToggled(this.props.span.spanID);
};

_linksGetter = (items: { key: string, value: any }[], itemIndex: number) => {
const { linksGetter, spanIndex } = this.props;
return linksGetter ? linksGetter(spanIndex, items, itemIndex) : [];
};

render() {
const {
color,
columnDivision,
detailState,
isFilteredOut,
linksGetter,
logItemToggle,
logsToggle,
processToggle,
Expand All @@ -76,6 +84,7 @@ export default class SpanDetailRow extends React.PureComponent<SpanDetailRowProp
<div className="detail-info-wrapper" style={{ borderTopColor: color }}>
<SpanDetail
detailState={detailState}
linksGetter={linksGetter ? this._linksGetter : null}
logItemToggle={logItemToggle}
logsToggle={logsToggle}
processToggle={processToggle}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ describe('<SpanDetailRow>', () => {
const spanDetail = (
<SpanDetail
detailState={props.detailState}
linksGetter={null}
logItemToggle={props.logItemToggle}
logsToggle={props.logsToggle}
processToggle={props.processToggle}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import {
isErrorSpan,
spanContainsErredSpan,
} from './utils';
import getLinks from '../../../model/link-patterns';
import type { Accessors } from '../ScrollManager';
import type { Log, Span, Trace } from '../../../types';
import colorGenerator from '../../../utils/color-generator';
Expand Down Expand Up @@ -264,10 +265,13 @@ export class VirtualizedTraceViewImpl extends React.PureComponent<VirtualizedTra
return DEFAULT_HEIGHTS.detail;
};

linksGetter = (spanIndex: number, items: { key: string, value: any }[], itemIndex: number) =>
getLinks(this.props.trace, spanIndex, items, itemIndex);

renderRow = (key: string, style: Style, index: number, attrs: {}) => {
const { isDetail, span, spanIndex } = this.rowStates[index];
return isDetail
? this.renderSpanDetailRow(span, key, style, attrs)
? this.renderSpanDetailRow(span, spanIndex, key, style, attrs)
: this.renderSpanBarRow(span, spanIndex, key, style, attrs);
};

Expand Down Expand Up @@ -352,7 +356,7 @@ export class VirtualizedTraceViewImpl extends React.PureComponent<VirtualizedTra
);
}

renderSpanDetailRow(span: Span, key: string, style: Style, attrs: {}) {
renderSpanDetailRow(span: Span, spanIndex: number, key: string, style: Style, attrs: {}) {
const { spanID } = span;
const { serviceName } = span.process;
const {
Expand Down Expand Up @@ -380,10 +384,12 @@ export class VirtualizedTraceViewImpl extends React.PureComponent<VirtualizedTra
onDetailToggled={detailToggle}
detailState={detailState}
isFilteredOut={isFilteredOut}
linksGetter={this.linksGetter}
logItemToggle={detailLogItemToggle}
logsToggle={detailLogsToggle}
processToggle={detailProcessToggle}
span={span}
spanIndex={spanIndex}
tagsToggle={detailTagsToggle}
traceStartTime={trace.startTime}
/>
Expand Down
1 change: 1 addition & 0 deletions packages/jaeger-ui/src/constants/default-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export default deepFreeze(
dagMaxNumServices: FALLBACK_DAG_MAX_NUM_SERVICES,
menuEnabled: true,
},
linkPatterns: [],
tracking: {
gaID: null,
trackErrors: true,
Expand Down
Loading