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

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

Merged
merged 18 commits into from
Aug 3, 2018
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
33fcae9
Link patterns to make values in tags, processes and logs clickable
divdavem Jul 4, 2018
83c257e
Fixing failing test
divdavem Jul 5, 2018
1330d0c
Moving icons closer to the value to make them more visible
divdavem Jul 5, 2018
f305938
Adding some tests
divdavem Jul 5, 2018
7849922
Fixing misplaced "// eslint-disable-next-line react/no-danger" comment
divdavem Jul 5, 2018
efd44fd
Using ub-inline-block instead of manipulating HTML in KeyValuesTable
divdavem Jul 6, 2018
0901e53
Adding tests for createTestFunction
divdavem Jul 6, 2018
0544a62
Adding tests for getParameterInArray
divdavem Jul 6, 2018
858bb10
Adding tests for getParameterInAncestor
divdavem Jul 6, 2018
136c149
Adding test for SpanDetailRow
divdavem Jul 6, 2018
73db8e9
Adding test for callTemplate
divdavem Jul 6, 2018
a159459
Adding test for computeLinks
divdavem Jul 6, 2018
dd795d2
Changes following code review
divdavem Jul 25, 2018
9acf208
Adding tests for getLinks
divdavem Jul 26, 2018
68c5d05
Changes following code review
divdavem Aug 1, 2018
13d66cc
Using # instead of $ in link templates
divdavem Aug 1, 2018
6048d0a
Adding a reference to the parent span in the child spans
divdavem Aug 1, 2018
185c93e
Merging addSpanReferences with transfromTraceData and making span opt…
divdavem Aug 2, 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
Prev Previous commit
Next Next commit
Changes following code review 
Signed-off-by: David-Emmanuel Divernois <david-emmanuel.divernois@amadeus.com>
  • Loading branch information
@divdavem
divdavem committed Aug 1, 2018
commit 68c5d0502494c8dd21d3e5d35b7ac62de2045ec5
7 changes: 4 additions & 3 deletions ...s/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanDetail/AccordianKeyValues.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,21 +21,22 @@ import IoIosArrowRight from 'react-icons/lib/io/ios-arrow-right';

import * as markers from './AccordianKeyValues.markers';
import KeyValuesTable from './KeyValuesTable';
import type { KeyValuePair, Link } from '../../../../types';

import './AccordianKeyValues.css';

type AccordianKeyValuesProps = {
className?: ?string,
data: { key: string, value: any }[],
data: KeyValuePair[],
highContrast?: boolean,
isOpen: boolean,
label: string,
linksGetter: ?({ key: string, value: any }[], number) => { url: string, text: string }[],
linksGetter: ?(KeyValuePair[], number) => Link[],
onToggle: () => void,
};

// export for tests
export function KeyValuesSummary(props: { data?: { key: string, value: any }[] }) {
export function KeyValuesSummary(props: { data?: KeyValuePair[] }) {
const { data } = props;
if (!Array.isArray(data) || !data.length) {
return null;
Expand Down
4 changes: 2 additions & 2 deletions packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanDetail/AccordianLogs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,13 @@ import IoIosArrowRight from 'react-icons/lib/io/ios-arrow-right';

import AccordianKeyValues from './AccordianKeyValues';
import { formatDuration } from '../utils';
import type { Log } from '../../../../types';
import type { Log, KeyValuePair, Link } from '../../../../types';

import './AccordianLogs.css';

type AccordianLogsProps = {
isOpen: boolean,
linksGetter: ?({ key: string, value: any }[], number) => { url: string, text: string }[],
linksGetter: ?(KeyValuePair[], number) => Link[],
logs: Log[],
onItemToggle: Log => void,
onToggle: () => void,
Expand Down
7 changes: 4 additions & 3 deletions packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanDetail/KeyValuesTable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import * as 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 type { KeyValuePair, Link } from '../../../../types';

import './KeyValuesTable.css';

Expand All @@ -37,7 +38,7 @@ const LinkValue = (props: { href: string, title?: string, children: React.Node }
</a>
);

const linkValueList = (links: { url: string, text: string }[]) => (
const linkValueList = (links: Link[]) => (
<Menu>
{links.map(({ text, url }, index) => (
// `index` is necessary in the key because url can repeat
Expand All @@ -50,8 +51,8 @@ const linkValueList = (links: { url: string, text: string }[]) => (
);

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

export default function KeyValuesTable(props: KeyValuesTableProps) {
Expand Down
4 changes: 2 additions & 2 deletions packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanDetail/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,13 @@ import AccordianLogs from './AccordianLogs';
import DetailState from './DetailState';
import { formatDuration } from '../utils';
import LabeledList from '../../../common/LabeledList';
import type { Log, Span } from '../../../../types';
import type { Log, Span, KeyValuePair, Link } from '../../../../types';

import './index.css';

type SpanDetailProps = {
detailState: DetailState,
linksGetter: ?({ key: string, value: any }[], number) => { url: string, text: string }[],
linksGetter: ?(KeyValuePair[], number) => Link[],
logItemToggle: (string, Log) => void,
logsToggle: string => void,
processToggle: string => void,
Expand Down
6 changes: 3 additions & 3 deletions packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanDetailRow.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import SpanDetail from './SpanDetail';
import DetailState from './SpanDetail/DetailState';
import SpanTreeOffset from './SpanTreeOffset';
import TimelineRow from './TimelineRow';
import type { Log, Span } from '../../../types';
import type { Log, Span, KeyValuePair, Link } from '../../../types';

import './SpanDetailRow.css';

Expand All @@ -30,7 +30,7 @@ type SpanDetailRowProps = {
detailState: DetailState,
onDetailToggled: string => void,
isFilteredOut: boolean,
linksGetter: ?(number, { key: string, value: any }[], number) => { url: string, text: string }[],
linksGetter: ?(number, KeyValuePair[], number) => Link[],
logItemToggle: (string, Log) => void,
logsToggle: string => void,
processToggle: string => void,
Expand All @@ -47,7 +47,7 @@ export default class SpanDetailRow extends React.PureComponent<SpanDetailRowProp
this.props.onDetailToggled(this.props.span.spanID);
};

_linksGetter = (items: { key: string, value: any }[], itemIndex: number) => {
_linksGetter = (items: KeyValuePair[], itemIndex: number) => {
const { linksGetter, spanIndex } = this.props;
return linksGetter ? linksGetter(spanIndex, items, itemIndex) : [];
};
Expand Down
4 changes: 2 additions & 2 deletions packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/VirtualizedTraceView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import {
} from './utils';
import getLinks from '../../../model/link-patterns';
import type { Accessors } from '../ScrollManager';
import type { Log, Span, Trace } from '../../../types';
import type { Log, Span, Trace, KeyValuePair } from '../../../types';
import colorGenerator from '../../../utils/color-generator';

import './VirtualizedTraceView.css';
Expand Down Expand Up @@ -265,7 +265,7 @@ export class VirtualizedTraceViewImpl extends React.PureComponent<VirtualizedTra
return DEFAULT_HEIGHTS.detail;
};

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

renderRow = (key: string, style: Style, index: number, attrs: {}) => {
Expand Down
55 changes: 27 additions & 28 deletions packages/jaeger-ui/src/model/link-patterns.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,26 +16,39 @@

import _uniq from 'lodash/uniq';
import { getConfigValue } from '../utils/config/get-config';
import type { Span, Trace } from '../types';
import type { Span, Trace, Link, KeyValuePair } from '../types';

const parameterRegExp = /\$\{([^{}]*)\}/g;

type ProcessedTemplate = {
parameters: string[],
template: (data: { [parameter: string]: any }) => string,
template: ({ [string]: mixed }) => string,
};

type ProcessedLinkPattern = {
object: any,
type: string => boolean,
key: string => boolean,
value: any => boolean,
url: ProcessedTemplate,
text: ProcessedTemplate,
parameters: string[],
};

function getParamNames(str) {
const names = [];
const names = new Set();
str.replace(parameterRegExp, (match, name) => {
names.push(name);
names.add(name);
return match;
});
return _uniq(names);
return Array.from(names);
}

function stringSupplant(str, encodeFn: any => string, map) {
return str.replace(parameterRegExp, (_, name) => encodeFn(map[name]));
return str.replace(parameterRegExp, (_, name) => {
const value = map[name];
return value == null ? '' : encodeFn(value);
});
}

export function processTemplate(template: any, encodeFn: any => string): ProcessedTemplate {
Expand Down Expand Up @@ -82,16 +95,6 @@ export function createTestFunction(entry: any) {

const identity = a => a;

type ProcessedLinkPattern = {
object: any,
type: string => boolean,
key: string => boolean,
value: any => boolean,
url: ProcessedTemplate,
text: ProcessedTemplate,
parameters: string[],
};

export function processLinkPattern(pattern: any): ?ProcessedLinkPattern {
try {
const url = processTemplate(pattern.url, encodeURIComponent);
Expand All @@ -112,7 +115,7 @@ export function processLinkPattern(pattern: any): ?ProcessedLinkPattern {
}
}

export function getParameterInArray(name: string, array: { key: string, value: any }[]) {
export function getParameterInArray(name: string, array: KeyValuePair[]) {
if (array) {
return array.find(entry => entry.key === name);
}
Expand Down Expand Up @@ -143,7 +146,7 @@ export function computeLinks(
linkPatterns: ProcessedLinkPattern[],
trace: Trace,
spanIndex: number,
items: { key: string, value: any }[],
items: KeyValuePair[],
itemIndex: number
) {
const item = items[itemIndex];
Expand All @@ -160,15 +163,15 @@ export function computeLinks(
const result = [];
linkPatterns.forEach(pattern => {
if (pattern.type(type) && pattern.key(item.key) && pattern.value(item.value)) {
let parameterValues = {};
pattern.parameters.every(parameter => {
const parameterValues = {};
const allParameters = pattern.parameters.every(parameter => {
let entry = getParameterInArray(parameter, items);
if (!entry && !processTags) {
Copy link
Member

Choose a reason for hiding this comment

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

I believe ancestors are also being searched when type === 'log', which seems to contradict your comment on the PR description?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, ancestors are also being searched when type === 'log', and this is on purpose.
Maybe I should update the PR description if it is ambiguous about this.

// do not look in ancestors for process tags because the same object may appear in different places in the hierarchy
Copy link
Member

Choose a reason for hiding this comment

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

Given this comment, why are process tags always searched when looking through ancestors? Seems like the only process tags that are skipped are those on the current span.

Also, why is it relevant that process tags can be repeated? A process will match the first time it's encountered or it will fail every time it's tested?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The getLinks function caches results in the alreadyComputed weak map.
The key used in that map is the object (with the key and value properties) that will be displayed as a link (if a link has to be displayed).
If that object is reused at different places in the hierarchy (which is the case for process tags, as the same process object can be reused for multiple spans), a call to getLinks with an object that is already in the cache could get the result of a previous call to getLinks from a different place in the hierarchy. If a process tag could use values from an ancestor, the ancestor from which the value could be found and the corresponding value could be different between the two places in the hierarchy, which would make getLinks return an invalid value.
Another solution (instead of preventing process tags links from using values from ancestors) would be to have a cache that also stores the current span (so we would need a cache with 2 keys), but that would be more complex and I am not sure if it is actually needed to refer to ancestor values from a process tag.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, having trouble following... Seems like items are only added to the cache in the link getter, at the outer level, and the cache is only checked at that level, too. So, when searching ancestors, previously calculated values that are cached are a non-issue?

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 will explain in Javascript:

// Let's say there is one tag like this:

let myProcessTag = {
  key: 'myProcessInfo',
  value: 'myProcessValue'
};

// The tag is used in a process:

let myProcess = {
  tags: [myProcessTag]
};

// The same process reference is used in multiple spans in the trace hierarchy:
let trace = {
  spans: [{
    depth: 0
  }, {
    depth: 1,
    tags: [{
      key: 'myParentKey',
      value: 'parentValue1'
    }]
  }, {
    depth: 2,
    process: myProcess
  }, {
    depth: 1,
    tags: [{
      key: 'myParentKey',
      value: 'parentValue2'
    }]
  }, {
    depth: 2,
    process: myProcess
  }]
};

// So myProcess is used in trace.spans[2], which is the child of trace.spans[1]
// and myProcess is also used in trace.spans[4], which is the child of trace.spans[3]

// Now, let's suppose that it is allowed for a link of a process tag to refer to tags from ancestors
// and someone defined the configuration like this:
JAEGER_CONFIG = {
  "linkPatterns": [{
    "key": "myProcessInfo",
    "url": "http://example.org/?myProcessValue=#{myProcessInfo}&myParentValue=#{myParentKey}",
    "text": "Where will this link go?"
  }]
};

// This link on the process tag refers to a tag from an ancestor
// (which is the thing I am not allowing in the current code).

// Then, if we call computeLinks with trace.spans[2] and myProcessTag, we should get:
[{
  "url": "http://example.org/?myProcessValue=myProcessValue&myParentValue=parentValue1",
  "text": "Where will this link go?"
}]

// Now, if we call computeLinks with trace.spans[4] and the same myProcessTag, we should get:
[{
  "url": "http://example.org/?myProcessValue=myProcessValue&myParentValue=parentValue2",
  "text": "Where will this link go?"
}]

// These two values are different because the same tag is at two different places in the spans hierarchy,
// and the value for myParentKey is taken
// - in the first case from trace.spans[1].tags[0]
// - in the second case from trace.spans[3].tags[0]

// Now the getLink function returned by createGetLinks uses the tag object as a key in the cache.
// In our case, the tag object is myProcessTag.

// So if we first call the getLink function returned by createGetLinks for trace.spans[2] and myProcessTag :
result = cache.get(myProcessTag); // result is undefined
result = computeLinks(...);
// which means:
result = [{
  "url": "http://example.org/?myProcessValue=myProcessValue&myParentValue=parentValue1",
  "text": "Where will this link go?"
}];
cache.set(myProcessTag, result); // result is cached

// now if we call the getLink function for trace.spans[4] and myProcessTag :
result = cache.get(myProcessTag); // result is already computed
return [{
  "url": "http://example.org/?myProcessValue=myProcessValue&myParentValue=parentValue1",
  "text": "Where will this link go?"
}]; // now the return value is wrong for trace.spans[4]

I hope this explanation is clearer!
I do not have the requirement of having links on process tags that use values from ancestor spans, so I chose the simplest solution of not allowing it. Do you think it would be better (for consistency or some use cases that I don't know) to allow this and then to use as the key in the cache both the span and the tag object (for example, with a cache based on nested WeakMaps)?

Copy link
Member

Choose a reason for hiding this comment

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

Great explanation, I think I've got it... Don't look in ancestors for process tags because the same process object, and therefore tags, can be used for many spans. In this case, searching a span's ancestors could yield different results (if they weren't cached). But, as the cache is keyed on the initial, shared, process tag, all spans with that process would end up getting the same results even if they have different ancestors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly!

// and the cache in getLinks uses that object as a key
entry = getParameterInAncestor(parameter, trace.spans, spanIndex);
}
if (entry && parameterValues) {
if (entry) {
parameterValues[parameter] = entry.value;
return true;
}
Expand All @@ -177,10 +180,9 @@ export function computeLinks(
`Skipping link pattern, missing parameter ${parameter} for key ${item.key} in ${type}.`,
pattern.object
);
parameterValues = null;
return false;
});
if (parameterValues) {
if (allParameters) {
result.push({
url: callTemplate(pattern.url, parameterValues),
text: callTemplate(pattern.text, parameterValues),
Expand All @@ -191,11 +193,8 @@ export function computeLinks(
return result;
}

export function createGetLinks(
linkPatterns: ProcessedLinkPattern[],
cache: WeakMap<{ key: string, value: any }, { url: string, text: string }[]>
) {
return (trace: Trace, spanIndex: number, items: { key: string, value: any }[], itemIndex: number) => {
export function createGetLinks(linkPatterns: ProcessedLinkPattern[], cache: WeakMap<KeyValuePair, Link[]>) {
return (trace: Trace, spanIndex: number, items: KeyValuePair[], itemIndex: number) => {
if (linkPatterns.length === 0) {
return [];
}
Expand Down
7 changes: 6 additions & 1 deletion packages/jaeger-ui/src/types/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,16 @@
* All timestamps are in microseconds
*/

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

export type Link = {
url: string,
text: string,
};

export type Log = {
timestamp: number,
fields: Array<KeyValuePair>,
Expand Down