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 optional tag actions #759

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

yoave23
Copy link
Contributor

@yoave23 yoave23 commented May 26, 2021

Which problem is this PR solving?

Short description of the changes

  • Add the ability to configure custom actions for tags, services and processes by adding a function to the UI-config
  • Add a configurable link to the link patterns config section
Screen.Recording.2021-05-26.at.14.08.54.mov

config used in the example:

"tagsActions": [{
          title: 'this link was configured in the ui-config',
          action: function(tag, span) {
            const { key, value } = tag;
            const url = `/search?service=${span.process.serviceName}&tags={"${key}":"${value}"}`;

            window.open(url);
          }
        },{
          title: 'display an alert',
          icon: 'setting',
          action: function(tag, span) {
            alert(JSON.stringify(span))
          }
        }]

@codecov
Copy link

codecov bot commented May 26, 2021

Codecov Report

Merging #759 (9a9ab5d) into master (c618bf6) will decrease coverage by 0.12%.
The diff coverage is 58.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #759      +/-   ##
==========================================
- Coverage   95.10%   94.98%   -0.13%     
==========================================
  Files         230      230              
  Lines        7010     7021      +11     
  Branches     1745     1749       +4     
==========================================
+ Hits         6667     6669       +2     
- Misses        337      346       +9     
  Partials        6        6              
Impacted Files Coverage Δ
...mponents/TracePage/TraceTimelineViewer/SpanBar.tsx 100.00% <ø> (ø)
...e/TraceTimelineViewer/SpanDetail/AccordianText.tsx 100.00% <ø> (ø)
...TracePage/TraceTimelineViewer/SpanDetail/index.tsx 100.00% <ø> (ø)
packages/jaeger-ui/src/model/link-patterns.tsx 92.70% <ø> (-1.86%) ⬇️
.../TraceTimelineViewer/SpanDetail/KeyValuesTable.tsx 85.36% <44.44%> (-11.70%) ⬇️
...ceTimelineViewer/SpanDetail/AccordianKeyValues.tsx 100.00% <100.00%> (ø)
...e/TraceTimelineViewer/SpanDetail/AccordianLogs.tsx 100.00% <100.00%> (ø)
...nents/TracePage/TraceTimelineViewer/SpanBarRow.tsx 94.44% <0.00%> (-5.56%) ⬇️

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 c618bf6...9a9ab5d. Read the comment docs.

@yurishkuro
Copy link
Member

yurishkuro commented May 26, 2021

Please make sure all commits are signed per contributing guide.

I like the functionality, except for the legend:
image
I think the legend is misplaced since it implies that the end user of the UI can actually go and configure the links, which is not the case in a typical enterprise setup.

I think this might also need a future extension to allow setting some filters on the individual tag actions, to be able to control which tags they should apply to. In fact, could we avoid introducing a new top-level section to the config but instead integrate this into the existing LinkPatterns configuration, which already supports selection of tags? We could extend it to make the selection optional or "*"

@yoave23
Copy link
Contributor Author

yoave23 commented Jul 29, 2021

@yurishkuro sorry for getting back to this PR so late

I like the functionality, except for the legend:
I think the legend is misplaced since it implies that the end user of the UI can actually go and configure the links, which is not the case in a typical enterprise setup.

this is a link to the link-patterns docs which can be overridden in the config

I think this might also need a future extension to allow setting some filters on the individual tag actions, to be able to control which tags they should apply to. In fact, could we avoid introducing a new top-level section to the config but instead integrate this into the existing LinkPatterns configuration, which already supports selection of tags? We could extend it to make the selection optional or "*"

we can avoid another top-level section in the config and use a "select all" option such as "*".
please note that we only display those links when a user hovers the tag as opposed to other links which are always displayed.

<div>
<a href={linkPatternsUrl} target="_blank" rel="noopener noreferrer">
<Icon type="info-circle-o" className="SpanDetail--docsIcon" />
Learn how to configure links
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this being shown within every span?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is, any other idea would be appreciated :)
we can also remove that part from this PR and do it in a separate one

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be too much to have this as part of every single span in a trace. We might have hundreds/thousands of them...

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, we'll try to find a better location for it or just remove it from this PR

Copy link
Member

Choose a reason for hiding this comment

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

As I already wrote earlier, this information shouldn't be in the UI because the end user of the UI cannot change the configuration, only the operator can.

@jpkrohling
Copy link
Contributor

@esnible, what do you think about this from a UX perspective?

@yurishkuro
Copy link
Member

@yoave23 are you still interested in finishing this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

improve the UX and discoverability of the link patterns
3 participants