Add links to make values in tags or log properties clickable

[divdavem]

This pull request contains an implementation of the feature described in #216.

It allows to define in the configuration some patterns so that the values of keys from the Tags, Process and Logs sections become clickable links that open the configured URL.

This has to be configured in the new linkPatterns property. For example:

{
  "linkPatterns": [{
    "type": "process",
    "key": "jaeger.version",
    "url": "https://github.com/jaegertracing/jaeger-client-go/releases/tag/#{jaeger.version}",
    "text": "Information about Jaeger release #{jaeger.version}"
  }]
}

When this is declared in the configuration, every time the jaeger.version key appears in the process section, the value of that key will be displayed as a clickable link, with an icon on the right to show that the value is clickable, for example:

If several patterns match the same tag, clicking on the value will display a popup with all the links:

Links are opened in a new window or tab.

In the configuration of a link, there are two kinds of properties:

• test properties: they determine whether the link should be displayed. These properties can contain a string or an array of strings specifying the possible values of that property that lead to the display of the link. All test properties must match for the link to be displayed. If a test property is not defined, it is considered to match.

  • type (possible values: process, logs or tags)
  • key
  • value

• template properties: they determine the url and text of the link. They are simple templates where each occurrence of #{key} is replaced by the value of the given key.

  • url: each replacement in the URL is wrapped in a call to encodeURIComponent
  • text: if there is only one link, the text of the link is displayed as a tooltip, if there are multiple links the text is displayed in the popup menu

  Template properties can depend on the value of the following keys:

  • for type process, only the keys in the current process can be used
  • for other types, the search order is the following:
    • for type logs: the keys present in the log
    • the keys of the tags or process in the span on which the log or tag being rendered can be found
    • the keys of the tags or process in the parent span according to the CHILD_OF hierarchy
    • other parents in the CHILD_OF hierarchy...

Commented extra features

Note that this PR originally also implemented the following possibilities that rely on the implementation of #123 (which allows the use of any JS expression instead of only JSON in the configuration). Those features have been commented in the code of the PR.

• test properties can contain functions or regular expressions instead of containing only strings or arrays of strings
• template properties can be implemented as an object containing two properties:
  • parameters: an array of strings specifying which keys are needed
  • template: a function that receives as its only argument an object with the keys declared in the parameters array and returns the url or the text of the link

For example, once #123 is implemented and the corresponding code is uncommented, the previous example can also be configured in the following equivalent way (using functions):

const JAEGER_CONFIG = {
  linkPatterns: [{
    type: (type) => type === "process",
    key: (key) => key === "jaeger.version",
    url: {
      parameters: ["jaeger.version"],
      template: data => `https://github.com/jaegertracing/jaeger-client-go/releases/tag/${encodeURIComponent(data["jaeger.version"])}`
    },
    text: {
      parameters: ["jaeger.version"],
      template: data => `Information about Jaeger release ${data["jaeger.version"]}`
    }
  }]
}

Thank you in advance for your comments.

[yurishkuro]

When are the evaluations performed, only when the span is expanded?

I think we'll need some visual indication that the tag/value have links attached, maybe the hyperlink 🔗 symbol somewhere.

[divdavem]

@yurishkuro Thank you for your answer.

When are the evaluations performed, only when the span is expanded?

Yes, exactly, just before links are displayed.

I think we'll need some visual indication that the tag/value have links attached, maybe the hyperlink 🔗 symbol somewhere.

There are already some icons on the right, but maybe they are not visible enough or not the right ones?
I have used icons from antd: the profile icon when there are multiple links and the export icon when there is a single link. I can change them if you think it is better .
The link icon usually means there is an anchor to link back to this position in the page, which is not exactly the case here, but maybe it is clearer than the icons I have used.

[codecov]

Codecov Report

Merging #223 into master will increase coverage by 0.92%.
The diff coverage is 93.13%.

@@            Coverage Diff             @@
##           master     #223      +/-   ##
==========================================
+ Coverage   89.21%   90.13%   +0.92%     
==========================================
  Files         106      109       +3     
  Lines        2317     2464     +147     
  Branches      472      505      +33     
==========================================
+ Hits         2067     2221     +154     
+ Misses        211      205       -6     
+ Partials       39       38       -1

Impacted Files	Coverage Δ
packages/jaeger-ui/src/constants/default-config.js	100% <ø> (ø)	⬆️
...cePage/TraceTimelineViewer/VirtualizedTraceView.js	93.85% <0%> (-0.84%)	⬇️
...ckages/jaeger-ui/src/model/transform-trace-data.js	82.6% <100%> (+3.12%)	⬆️
packages/jaeger-ui/src/model/span.js	100% <100%> (ø)
.../TracePage/TraceTimelineViewer/SpanDetail/index.js	100% <100%> (ø)	⬆️
...aceTimelineViewer/SpanDetail/AccordianKeyValues.js	100% <100%> (ø)	⬆️
...nts/TracePage/TraceTimelineViewer/SpanDetailRow.js	100% <100%> (ø)	⬆️
...ge/TraceTimelineViewer/SpanDetail/AccordianLogs.js	100% <100%> (ø)	⬆️
packages/jaeger-ui/src/model/link-patterns.js	92.85% <92.85%> (ø)
...e/TraceTimelineViewer/SpanDetail/KeyValuesTable.js	86.36% <93.33%> (+8.58%)	⬆️
... and 11 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa0f161...185c93e. Read the comment docs.

[divdavem]

I have added a commit to make icons more visible:

• they are now just next to the value, on the right
• they are in bold

Also, when there are multiple links, the popup is no longer on the whole column width, but has a width adapted to its content.

[yurishkuro]

+1

I would stick with
this icon:

One concern with showing it on the right is when the value of the tag is long & truncated.

[tiffon]

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;
}

[divdavem]

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

[tiffon]

@divdavem, Awesome! 🎉 Thank you for putting this together! :)

I have a few questions but want to spend more time digesting it and should be able to follow up tomorrow.

[divdavem]

@tiffon Thank you. For info, I am on holidays tonight for a bit more than two weeks, so I may not be able to answer your questions soon. I am planning to add some more tests this afternoon before my holidays.

[divdavem]

There seems to be a failing test in Travis which is unrelated to my changes: https://travis-ci.org/jaegertracing/jaeger-ui/builds/400936060#L1378

Summary of all failing tests
FAIL src/selectors/trace.test.js (122.854s)
● getTraceServiceCount() should return the length of the service list
expect(received).toBe(expected)

Expected value to be (using ===):
  5
Received:
  6
  
  at Object.<anonymous>.it (src/selectors/trace.test.js:168:63)
  at Promise.resolve.then.el (../../node_modules/p-map/index.js:46:16)
  at process._tickCallback (internal/process/next_tick.js:109:7)

Between the commit of the previous successful build (https://travis-ci.org/jaegertracing/jaeger-ui/builds/400922649) (136c149) and the commit of this failing travis build (a159459), I only added some tests and did not change any other code: divdavem/jaeger-ui@136c149...a159459

The data used in the failing test seem to be randomly generated, that's probably why such a failure suddenly happened in one build and does not happen in most builds, but, it may underline a real issue, either in the random data generation or in the code.

[tiffon]

Thanks, @divdavem! 👍

Some questions and comments added inline to the code.

Regarding typing, not all of the code-base has flow typings, some older files are missing them, but we're trying to use flow whenever we add new files or heavily refactor. Can you add create flow types in src/types/link-patterns.js and use them throughout the typed code?

Also, I think it makes sense to add the spanIndex prop to the SpanDetailRow, but a couple of things that crossed my mind:

• It might be nice to avoid passing the full trace (or even just it's spans) into getLinks(...)
• It might be nice for spans to have a real reference to it's parent instead of just the ID (not sure yet about the full implications of this)

Just thinking out loud about the possibility of adding a .parent property to Span. Then, getLinks(...) could be:

getLinks(span, items, itemIndex)`

What do you think?

Thank you for contributing!

[tiffon]

For the string interpolation, what do you think of using something like twitter-text/stringSupplant?

So, to get the parameter names, something like:

function getParamNames(str) {
  const names = [];
  str.replace(/#\{([^\}]+)\}/g, function (_, name) {
    names.push(name);
  });
  return names;
}

And to apply them:

function stringSupplant(str, encodeFn, map) {
  return str.replace(/#\{([^}]+)\}/g, function (_, name) {
    const v = map[name];
    return v ? encodeFn(v) : '';
  });
}

I think this would simplify processTemplate() and the resulting template() function.

template: stringSupplant.bind(null, template, endcodeFn),

[divdavem]

Your suggestion is clearly more readable, even if it requires to execute the regular expression each time the template is executed (as opposed to only once, when processTemplate is called). The difference in performance (if any) is probably negligible, so I have updated the PR to use your suggestion.
Do you have any preference between #{variable} or ${variable} ?

[tiffon]

Thank you.

I'd suggest using the # to differentiate from template literals.

[divdavem]

Ok. I have updated the PR to use # instead of $.

[tiffon]

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?

[divdavem]

I have now commented the parts of the code that rely on #123.

[tiffon]

This will not exit early when a root level ancestor is encountered, e.g.

for (...) {
  ...
  if (currentSpan.depth === 0) {
    break; // as there will be no more ancestors
  }
}

[divdavem]

I have now added this condition in the for loop to exit early when reaching a root level ancestor.

[tiffon]

Why is pattern.key(...) getting item.value and type?

[divdavem]

This allowed to have a function that can decide whether to display a link with a logic depending on those multiple parameters. I have now removed that possibility as it also depends on #123.

[tiffon]

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

[divdavem]

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.

[tiffon]

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?

[divdavem]

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.

[tiffon]

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?

[divdavem]

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)?

[tiffon]

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?

[divdavem]

Exactly!

[tiffon]

This can be shortened to .filter(Boolean).

[divdavem]

Thank you! I have updated the PR.

[tiffon]

This file looks great! Awesome.

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

[divdavem]

Thank you for your encouraging feedback!

[tiffon]

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?

[divdavem]

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(...)}).

[tiffon]

Looks great, thank you.

[divdavem]

@tiffon Thank you for your review!

Now that I am back from my holidays, I have updated the PR to take your remarks into account.

To answer your question about the model, it is true that it would make the code a bit simpler to have a reference to the parent span from a child span. As you say, it would avoid having to pass the full trace and the index in the trace. Moreover it would allow getParameterInAncestor to be more efficient as it would not have to loop over (and skip) spans that are not ancestors.
However, I don't know the implications of this change, as it obviously makes the model recursive (preventing it from being serialized easily...).

[tiffon]

@divdavem This looks great! Thank you for the changes, very thoughtful descriptions and explanations, and consideration of the parent reference!

I will think more about parent references and dive further into the changes, Monday.

Side note: This is the biggest PR in Jaeger UI from someone who doesn't work for Uber (by a landslide)... MILESTONE! Thank you!

[tiffon]

Looks great, thank you.

[tiffon]

This could maybe use a Set?

const names = new Set();
// ...
return Array.from(names);

[divdavem]

Ok

[tiffon]

I think this can be

template: { [string]: any } => string,

[tiffon]

Also, would mixed fit the bill instead of any?

https://flow.org/en/docs/types/mixed/

[divdavem]

Ok! I do not know flow very well. I mostly use Typescript.

[tiffon]

encodeUriComponent will stringify null (and undefined?) values, so maybe it makes sense to guard the encode function invocation?

encodeURIComponent(null)
// -> "null"

fn = () => null;
'abbc'.replace(/b/g, fn)
// -> "anullnullc"

[divdavem]

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.

[tiffon]

Thank you!

[tiffon]

Can you please move type definitions to the top of the file?

[divdavem]

Ok!

[tiffon]

Thank you.

I'd suggest using the # to differentiate from template literals.

[tiffon]

Your changes look great. I added minor comments, but did ask about the process tags vs caching issue that you mentioned.

Also, after thinking it over, seems like it makes sense to add a reference to the parent. It's semantically consistent with the existing model (as opposed to adding references to children), but more convenient than jsut having the span ID. Futher, process objects are already referenced among multiple spans.

Adding a span property to the SpanReference type and using a utility function to get the parent of the span shoul work.

Existing

jaeger-ui/packages/jaeger-ui/src/types/index.js

Lines 35 to 40 in 0188418

	export type SpanReference = {
	refType: 'CHILD_OF' | 'FOLLOWS_FROM',
	spanID: string,
	traceID: string,
	};

With the parent added as an actual reference via the SpanReference object

export type SpanReference = {
  refType: 'CHILD_OF' | 'FOLLOWS_FROM',
  span: Span,
  spanID: string,
  traceID: string,
};

A util function to make life easier when getting the parent could live in src/model/span.js (I'm going to be removing the selectors folder, at some point)

export function getParent(span: Span) {
  // searches the span.references to find 'CHILD_OF' reference type or returns null
}

What do you think?

[tiffon]

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

jaeger-ui/packages/jaeger-ui/src/types/index.js

Lines 21 to 24 in 0188418

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

[divdavem]

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.

[tiffon]

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?

[tiffon]

Very nicely done. A consideration from a reader's perspective, using the return value of the .every() instead of setting parameterValues to null might be easier to follow.

const ok = pattern.parameters.every(parameter => {
  // ...
});

if (ok) {
  result.push(...);
}

[divdavem]

Indeed, I did not think about using the return value, but that is a good idea, thank you!

[divdavem]

@tiffon Thank you for your review!
I have added a commit to add references to the parent span in the model. It simplifies the code in many places (allowing to simply pass a span instead of passing a trace and a span index), and it makes sense, but it also adds two loops of processing in addSpanReferences called at the end of transfromTraceData.
If there is at most one reference per span, and if that reference is of type CHILD_OF or FOLLOWS_FROM (as it seems to be assumed in getTraceSpanIdsAsTree in trace.js), it would be possible to merge those two loops together (because in that case, the spans referred to are always before the spans containing the reference, in the spans array). Also, maybe it would be good to do that as part of the existing loop in transfromTraceData, or is it better to keep things separate (which is, maybe, a bit easier to read)?
What do you think?

[tiffon]

@divdavem – Awesome, looks great!

Re the parent span reference, one small suggestion. I'd say you can hook into the tree traversal and add the span pointer then. When tree.walk(...) is done to order the spans, the spanMap is fully populated, so the referred to spans can be accessed from there.

jaeger-ui/packages/jaeger-ui/src/model/transform-trace-data.js

Lines 79 to 86 in 0188418

	tree.walk((spanID, node, depth) => {
	if (spanID === '__root__') {
	return;
	}
	const span: ?SpanWithProcess = spanMap.get(spanID);
	if (!span) {
	return;
	}

I think the type def might need to have span as ?Span, there are cases where the parent is MIA (in our instrumentations, at least). Also, it's possible for FOLLOWS_FROM references to refer to spans in different traces.

[tiffon]

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?

[divdavem]

@tiffon Thank you for your answer. I have pushed a commit to merge addSpanReferences with transfromTraceData and make span optional in SpanReference.

Note that I had to use a type cast to any to make flow accept my change (is there a better solution?). That's because I am converting a SpanWithProcess object into a Span object by adding the missing properties. I had to remove the creation of a new object so that spanMap (from which I get the reference to put in the SpanReference object) contains the same references as the final spans array (otherwise it contains old SpanWithProcess references before their copy in new Span objects).

[tiffon]

Looks great, thanks! Seems like the only alternative to any is to make a new Map from the final spans and collect the references into an array as the tree is walked. Then, the references can be updated to point to the span after, the fact. I think either way works.

LGTM I'm going to run this tomorrow and experiment around with it, then will stamp and merge. 👍

[divdavem]

Ok! Thank you!

[tiffon]

@divdavem I believe it's working, as expected.

One thing I noticed that I didn't expect: only the single kvp from the log item, span tag or process tag is searched on that span. If all parameters are not found on that particular key-value-pair, then the rest of the span (span tags, logs or process tags) is not searched. In the case of process link patterns, things end. In the case of logs or tags, the ancestors are searched. Is that right? I intuitively expected the rest of the span to be searched before the ancestors were searched.

Just curious about the rationale for not searching any further on that span? Is that part of your use-case?

I'm going to circle back to the team and see if anyone has any reflections on this aspect of the link construction...

[divdavem]

@tiffon Can you show an example of what you are surprised about (with the configuration and the sample trace)?

[tiffon]

@divdavem Yes, absolutely. Thank you for asking. I got my wires crossed a bit, I believe the rest of the list of key-value-pairs is searched, for instance, the other kvps in a log message, but aside from that the rest of the span is not searched.

For instance, if we have the following config (written in JavaScript but it would actually be in JSON):

linkPatters: [{
  "type": "logs",
  "key": "errorType",
  "url": "/find-more-logs?filterLevel=error&filterErrorType=#{errorType}&filterPhase=#{phase}&filterAuthResult=#{authenticationResult}&filterModus=#{modus}",
  "text": "This links to a search on logs for errors of the same type with the same authentication result"
}]

If we had the following span:

{
  spanID: 'abc',
  depth: 0,
  tags: [{
    { key: 'modus', value: 'operandi' },
  }]
  // etc...
  logs: [
    {
      timestamp: 999,
      fields: [
        { key: 'authenticationResult', value: 200 }
      ]
    }
    {
      timestamp: 1001,
      fields: [
        { key: 'error', value: true },
        { key: 'errorType', value: 'timeout' },
        { key: 'phase', value: 'startup' }
      ]
    }
  ]
}

I believe when the link pattern is processed it will not fill in the parameterValues because only the fields on the second log message will be checked. Not finding the authenticationResult or modus fields, ancestors would be searched, but there are none.

While parameterValues will not be fully populated and the link will not be generated, the example would be able to populate errorType and phase.

Initially, I incorrectly assumed phase would not be populated, but I see now that it would.

I was surprised that ancestor spans are searched but the rest of the initial span is not. If the logs of the initial span and the tags and process tags were checked, the link would be generated.

I'll check-in with the team this afternoon and will report back. Thanks!

[divdavem]

@tiffon Thank you for this example.
For logs, we do not have the requirement to look in sibling log entries, we only look in the set of key value pairs in the current log entry then we look in the current span that contains this log (tags and process tags, not other log entries) then we go to the parent span (with tags and process tags, not logs)...
So, in your example, authenticationResult would not be found (it is in a sibling log), but modus would (it is in the current span).

[tiffon]

@divdavem Thank you for the clarification! You're absolutely right, modus would be found.

Awesome!

Also, we're always ready to learn more about how folks are using Jaeger. LMK if you'd have some free time to discuss what's working and what's not for you and your team.

Thanks so much for contributing! 🏆 💯

[divdavem]

@tiffon Thank you very much for the time you spent discussing and reviewing this PR and for integrating it!

I personally do not have much experience with Jaeger, but, being part of a UI team, I developed this feature for another team that uses Jaeger and needed this improvement. I will suggest them to share their feedback with you.

Thank you again for welcoming so kindly external contributions!