Make traceID, startTime, endTime, duration and traceName available for Link Patterns #1178

MUI-Pop · 2023-02-04T18:40:14Z

Which problem is this PR solving?

This change makes trace related keys like traceID, traceName, startTime, endTime and duration be available for Link Patterns.

It partially solves part of #578

Short description of the changes

Pass 'trace' object as additional parameter to LinksPatterns GetLink, there by trace variable would be accessible when computing link patterns for process, tags and logs type.

…r process, tags and logs linkpatterns Signed-off-by: Kanahathi Mohideen <kpmfazal@gmail.com>

yurishkuro

Overall looks good, but I have some suggestions, some to address in this PR, and a larger scoped issue #1179

Q: why does the description say that "It partially solves part of #578"? Could you please list which aspects it does not solve?

yurishkuro · 2023-02-04T20:47:50Z

packages/jaeger-ui/src/model/link-patterns.test.js

  ].map(processLinkPattern);

  const spans = [
    { depth: 0, process: {}, tags: [{ key: 'myKey', value: 'valueOfMyKey' }] },
    { depth: 1, process: {}, logs: [{ fields: [{ key: 'myOtherKey', value: 'valueOfMy+Other+Key' }] }] },
+    { depth: 1, process: {}, tags: [{ key: 'myThirdKey', value: 'valueOfThirdMyKey' }] },


seem redundant adding a new span, just add a new tag to one of the other spans

Fixed - added a new to existing span.

yurishkuro · 2023-02-04T20:49:57Z

packages/jaeger-ui/src/model/link-patterns.test.js

+    {
+      type: 'tags',
+      key: 'myThirdKey',
+      url: 'http://example.com/?myKey1=#{myKey}&myKey=#{myThirdKey}&traceID=#{traceID}&startTime=#{startTime}',


I don't think it's a good idea that you can reference trace-level attributes without qualification. I can see it for traceID, but why does startTime refer to trace-level and not span-level?

My preference would be to use trace.traceID, trace.startTime, etc. to clearly indicate the scope, not to try to infer it automatically.

Reading more into the existing code I am not a big fan of how it automatically ascends into ancestor spans when looking for tags. #1179

Fixed - now trace level are accessible only with trace.<data> qualification in template variables.

packages/jaeger-ui/src/model/link-patterns.tsx

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/VirtualizedTraceView.tsx

MUI-Pop · 2023-02-05T18:49:50Z

I said it partial because the current change makes trace related data accessible to logs, process and tags link pattern types but not span related data.

Signed-off-by: Kanahathi Mohideen <kpmfazal@gmail.com>

codecov · 2023-02-06T01:04:06Z

Codecov Report

Base: 95.26% // Head: 95.47% // Increases project coverage by +0.20% 🎉

Coverage data is based on head (4991ce3) compared to base (0395ce1).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1178      +/-   ##
==========================================
+ Coverage   95.26%   95.47%   +0.20%     
==========================================
  Files         243      243              
  Lines        7562     7575      +13     
  Branches     1895     1898       +3     
==========================================
+ Hits         7204     7232      +28     
+ Misses        351      336      -15     
  Partials        7        7

Impacted Files	Coverage Δ
...ePage/TraceTimelineViewer/VirtualizedTraceView.tsx	`96.66% <100.00%> (+0.72%)`	⬆️
packages/jaeger-ui/src/model/link-patterns.tsx	`95.41% <100.00%> (+0.51%)`	⬆️
...mponents/TracePage/TraceStatistics/tableValues.tsx	`98.62% <0.00%> (+4.82%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Signed-off-by: Kanahathi Mohideen <kpmfazal@gmail.com>

packages/jaeger-ui/src/model/link-patterns.tsx

yurishkuro · 2023-02-07T00:23:25Z

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/VirtualizedTraceView.test.js

+    it('calls getLinks with expected params', () => {
+      const span = trace.spans[1];
+      instance.linksGetter(span, span.tags, 0);
+      expect(getLinksSpy).toHaveBeenCalledWith(span, span.tags, 0, trace);


it would be better to validate the actual value returned, which, as I understand, is [{url, text}]

I didn't find a clean way to initialize linkPatterns for tests, so I exported the const processedLinks in link-patterns.tsx.

Signed-off-by: Kanahathi Mohideen <kpmfazal@gmail.com>

yurishkuro

lgtm, just a couple minor comments.

I didn't find a clean way to initialize linkPatterns for tests, so I exported the const processedLinks in link-patterns.tsx.

yeah, the API feels unnecessarily complicated, it could've just accepted processed patterns as optional arg, since the methods to process patterns are exported anyway.

yurishkuro · 2023-02-07T22:05:50Z

packages/jaeger-ui/src/model/link-patterns.tsx

+          const traceKV = getParameterInTrace(parameter.split('trace.')[1], trace);
+          if (traceKV) {
+            entry = traceKV;
+          }


Suggested change

const traceKV = getParameterInTrace(parameter.split('trace.')[1], trace);

if (traceKV) {

entry = traceKV;

}

entry = getParameterInTrace(parameter.split('trace.')[1], trace);

yurishkuro · 2023-02-07T22:13:12Z

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/VirtualizedTraceView.test.js

+      getLinks.processedLinks.push(...origLinkPatterns);
+    });
+
+    it('calls getLinks with expected params', () => {


stale title

yurishkuro · 2023-02-07T22:16:09Z

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/VirtualizedTraceView.test.js

@@ -22,6 +22,7 @@ import { DEFAULT_HEIGHTS, VirtualizedTraceViewImpl } from './VirtualizedTraceVie
 import traceGenerator from '../../../demo/trace-generators';
 import transformTraceData from '../../../model/transform-trace-data';
 import updateUiFindSpy from '../../../utils/update-ui-find';
+import * as getLinks from '../../../model/link-patterns';


nit

Suggested change

import * as getLinks from '../../../model/link-patterns';

import * as linkPatterns from '../../../model/link-patterns';

Signed-off-by: Kanahathi Mohideen <kpmfazal@gmail.com>

yurishkuro · 2023-02-08T05:43:02Z

Thanks! 🎉

Could you also make a PR in the documentation repo to describe this change in https://www.jaegertracing.io/docs/1.42/frontend-ui/#link-patterns ?

Updating the documentation for the linkPatterns change done in jaeger ui in PR jaegertracing/jaeger-ui#1178 Signed-off-by: Kanahathi Mohideen <kpmfazal@gmail.com>

MUI-Pop requested review from everett980, rubenvp8510 and tiffon as code owners February 4, 2023 18:40

Make traceID, startTime, endTime, duration and traceName available fo…

0318def

…r process, tags and logs linkpatterns Signed-off-by: Kanahathi Mohideen <kpmfazal@gmail.com>

MUI-Pop force-pushed the inject-trace-details-to-linkpatterns branch from ff9f39f to 0318def Compare February 4, 2023 18:43

yurishkuro reviewed Feb 4, 2023

View reviewed changes

MUI-Pop added 2 commits February 5, 2023 13:53

Fix review comments

adf1e94

Signed-off-by: Kanahathi Mohideen <kpmfazal@gmail.com>

Fix lint errors

4b74c6d

Signed-off-by: Kanahathi Mohideen <kpmfazal@gmail.com>

Add tests for VirtualizedTraceView

c4e42e1

Signed-off-by: Kanahathi Mohideen <kpmfazal@gmail.com>

MUI-Pop requested review from yurishkuro and removed request for tiffon, everett980 and rubenvp8510 February 6, 2023 21:19

yurishkuro reviewed Feb 7, 2023

View reviewed changes

Fix review comments

d3980bc

Signed-off-by: Kanahathi Mohideen <kpmfazal@gmail.com>

yurishkuro previously approved these changes Feb 7, 2023

View reviewed changes

Address review comments

4d507da

Signed-off-by: Kanahathi Mohideen <kpmfazal@gmail.com>

MUI-Pop dismissed yurishkuro’s stale review via 4d507da February 8, 2023 03:42

yurishkuro approved these changes Feb 8, 2023

View reviewed changes

Merge branch 'main' into inject-trace-details-to-linkpatterns

4991ce3

yurishkuro merged commit e2297dc into jaegertracing:main Feb 8, 2023

MUI-Pop mentioned this pull request Feb 8, 2023

Add documentation for jaeger ui PR#1178 jaegertracing/documentation#602

Merged

yurishkuro changed the title ~~Make traceID, startTime, endTime, duration and traceName available fo…~~ Make traceID, startTime, endTime, duration and traceName available for Link Patterns Mar 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make traceID, startTime, endTime, duration and traceName available for Link Patterns #1178

Make traceID, startTime, endTime, duration and traceName available for Link Patterns #1178

MUI-Pop commented Feb 4, 2023

yurishkuro left a comment

yurishkuro Feb 4, 2023

MUI-Pop Feb 5, 2023

yurishkuro Feb 4, 2023

yurishkuro Feb 4, 2023

MUI-Pop Feb 5, 2023

MUI-Pop commented Feb 5, 2023

codecov bot commented Feb 6, 2023 •

edited

Loading

yurishkuro Feb 7, 2023

MUI-Pop Feb 7, 2023

yurishkuro left a comment

yurishkuro Feb 7, 2023

yurishkuro Feb 7, 2023

yurishkuro Feb 7, 2023

yurishkuro commented Feb 8, 2023

	import * as getLinks from '../../../model/link-patterns';
	import * as linkPatterns from '../../../model/link-patterns';

Make traceID, startTime, endTime, duration and traceName available for Link Patterns #1178

Make traceID, startTime, endTime, duration and traceName available for Link Patterns #1178

Conversation

MUI-Pop commented Feb 4, 2023

Which problem is this PR solving?

Short description of the changes

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MUI-Pop commented Feb 5, 2023

codecov bot commented Feb 6, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Feb 8, 2023

codecov bot commented Feb 6, 2023 •

edited

Loading