Add links to make values in tags or log properties clickable #223
Signed-off-by: David-Emmanuel Divernois <david-emmanuel.divernois@amadeus.com>
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,5 @@ | ||
| // @flow | ||
|
|
||
| // Copyright (c) 2017 The Jaeger Authors. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
|
|
@@ -14,58 +16,67 @@ | |
|
|
||
| import _uniq from 'lodash/uniq'; | ||
| import { getConfigValue } from '../utils/config/get-config'; | ||
| import type { Span, Trace } from '../types'; | ||
|
|
||
| const parameterRegExp = /\$\{([^{}]*)\}/g; | ||
|
|
||
| type ProcessedTemplate = { | ||
| parameters: string[], | ||
| template: (data: { [parameter: string]: any }) => string, | ||
|
There was a problem hiding this comment. I think this can be There was a problem hiding this comment. Also, would There was a problem hiding this comment. Ok! I do not know flow very well. I mostly use Typescript. |
||
| }; | ||
|
|
||
| function getParamNames(str) { | ||
| const names = []; | ||
| str.replace(parameterRegExp, (match, name) => { | ||
| names.push(name); | ||
| return match; | ||
| }); | ||
| return _uniq(names); | ||
|
There was a problem hiding this comment. This could maybe use a |
||
| } | ||
|
|
||
| function stringSupplant(str, encodeFn: any => string, map) { | ||
| return str.replace(parameterRegExp, (_, name) => encodeFn(map[name])); | ||
|
There was a problem hiding this comment.
encodeURIComponent(null)
// -> "null"
fn = () => null;
'abbc'.replace(/b/g, fn)
// -> "anullnullc"There was a problem hiding this comment. Ok. As links are not displayed if any parameter is missing, this problem only happens when tags are explicitly added with a null or undefined value. In this case, it is true that is better to guard the encode function invocation and have an empty string rather than the strange "undefined" or "null" strings. I have updated the code. |
||
| } | ||
|
|
||
| export function processTemplate( | ||
| template: any, | ||
| encodeFn: any => string | ||
| ): ProcessedTemplate { | ||
| if (typeof template !== 'string') { | ||
| /* | ||
|
|
||
| // kept on ice until #123 is implemented: | ||
|
|
||
| if (template && Array.isArray(template.parameters) && (typeof template.template === 'function')) { | ||
| return template; | ||
| } | ||
|
|
||
| */ | ||
| throw new Error('Invalid template'); | ||
| } | ||
| return { | ||
| parameters: getParamNames(template), | ||
| template: stringSupplant.bind(null, template, encodeFn), | ||
| }; | ||
| } | ||
|
|
||
| export function createTestFunction(entry: any) { | ||
| if (typeof entry === 'string') { | ||
| return (arg: any) => arg === entry; | ||
| } | ||
| if (Array.isArray(entry)) { | ||
| return (arg: any) => entry.indexOf(arg) > -1; | ||
| } | ||
| /* | ||
|
|
||
| // kept on ice until #123 is implemented: | ||
| if (entry instanceof RegExp) { | ||
|
There was a problem hiding this comment. Planning ahead in preparation for the JS config file (#123) is awesome but it's tough to validate functionality that relies on or is impacted by the JS config before it's implemented. That increases the risks when switching to using the JS config. Maybe the regular expression and function variants of the test function should be kept on ice until a PR for #128 is merged? There was a problem hiding this comment. I have now commented the parts of the code that rely on #123. |
||
| return (arg: any) => entry.test(arg); | ||
| } | ||
| if (typeof entry === 'function') { | ||
| return entry; | ||
| } | ||
|
|
||
| */ | ||
| if (entry == null) { | ||
| return () => true; | ||
| } | ||
|
|
@@ -74,7 +85,17 @@ export function createTestFunction(entry) { | |
|
|
||
| const identity = a => a; | ||
|
|
||
| type ProcessedLinkPattern = { | ||
|
There was a problem hiding this comment. Can you please move type definitions to the top of the file? |
||
| 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); | ||
| const text = processTemplate(pattern.text, identity); | ||
|
|
@@ -94,16 +115,16 @@ export function processLinkPattern(pattern) { | |
| } | ||
| } | ||
|
|
||
| export function getParameterInArray(name: string, array: { key: string, value: any }[]) { | ||
| if (array) { | ||
| return array.find(entry => entry.key === name); | ||
| } | ||
| return undefined; | ||
| } | ||
|
|
||
| export function getParameterInAncestor(name: string, spans: Span[], startSpanIndex: number) { | ||
| let currentSpan = { depth: spans[startSpanIndex].depth + 1 }; | ||
| for (let spanIndex = startSpanIndex; spanIndex >= 0 && currentSpan.depth > 0; spanIndex--) { | ||
| const nextSpan = spans[spanIndex]; | ||
| if (nextSpan.depth < currentSpan.depth) { | ||
| currentSpan = nextSpan; | ||
|
|
@@ -117,11 +138,17 @@ export function getParameterInAncestor(name, spans, startSpanIndex) { | |
| return undefined; | ||
| } | ||
|
|
||
| function callTemplate(template, data) { | ||
| return template.template(data); | ||
| } | ||
|
|
||
| export function computeLinks( | ||
| linkPatterns: ProcessedLinkPattern[], | ||
| trace: Trace, | ||
| spanIndex: number, | ||
| items: { key: string, value: any }[], | ||
| itemIndex: number | ||
| ) { | ||
| const item = items[itemIndex]; | ||
| const span = trace.spans[spanIndex]; | ||
| let type = 'logs'; | ||
|
|
@@ -135,15 +162,16 @@ export function computeLinks(linkPatterns, trace, spanIndex, items, itemIndex) { | |
| } | ||
| const result = []; | ||
| linkPatterns.forEach(pattern => { | ||
| if (pattern.type(type) && pattern.key(item.key) && pattern.value(item.value)) { | ||
| let parameterValues = {}; | ||
|
There was a problem hiding this comment. Very nicely done. A consideration from a reader's perspective, using the return value of the There was a problem hiding this comment. Indeed, I did not think about using the return value, but that is a good idea, thank you! |
||
| pattern.parameters.every(parameter => { | ||
| let entry = getParameterInArray(parameter, items); | ||
| if (!entry && !processTags) { | ||
|
There was a problem hiding this comment. I believe ancestors are also being searched when type === 'log', which seems to contradict your comment on the PR description? There was a problem hiding this comment. Yes, ancestors are also being searched when |
||
| // do not look in ancestors for process tags because the same object may appear in different places in the hierarchy | ||
|
There was a problem hiding this comment. 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? There was a problem hiding this comment. The There was a problem hiding this comment. 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? There was a problem hiding this comment. 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! There was a problem hiding this comment. 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? |
||
| // and the cache in getLinks uses that object as a key | ||
| entry = getParameterInAncestor(parameter, trace.spans, spanIndex); | ||
| } | ||
| if (entry && parameterValues) { | ||
| parameterValues[parameter] = entry.value; | ||
| return true; | ||
| } | ||
|
|
@@ -166,10 +194,15 @@ export function computeLinks(linkPatterns, trace, spanIndex, items, itemIndex) { | |
| return result; | ||
| } | ||
|
|
||
| const linkPatterns = (getConfigValue('linkPatterns') || []).map(processLinkPattern).filter(Boolean); | ||
| const alreadyComputed = new WeakMap(); | ||
|
|
||
| export default function getLinks( | ||
| trace: Trace, | ||
| spanIndex: number, | ||
| items: { key: string, value: any }[], | ||
| itemIndex: number | ||
| ) { | ||
| if (linkPatterns.length === 0) { | ||
| return []; | ||
| } | ||
|
|
||
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.
This file looks great! Awesome.
(Not sure how to add a comment to a file instead of a line...)
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.
Thank you for your encouraging feedback!