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

Jump to bottom

Added jaeger ui version to about menu #606

Merged
merged 6 commits into from
Oct 18, 2020
Merged

Added jaeger ui version to about menu #606

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2a55237
Added jaeger ui version to about menu
alanisaac Jul 28, 2020
d5a4b0a
Merge branch 'master' into display-ui-version
alanisaac Oct 18, 2020
e412f77
Added Jaeger version info from config
alanisaac Oct 18, 2020
3d3a0b6
Removed extra 'v' from Jaeger version
alanisaac Oct 18, 2020
f7624bf
Changed to use short git commit hash
alanisaac Oct 18, 2020
9f5e7fe
Added test for missing coverage
alanisaac Oct 18, 2020
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
20 changes: 17 additions & 3 deletions packages/jaeger-ui/src/components/App/TopNav.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ describe('<TopNav>', () => {
const blogUrl = 'https://medium.com/jaegertracing/';
const labelAbout = 'About Jaeger';
const dropdownItems = [
{
label: 'Version 1',
},
{
label: 'Docs',
url: 'http://jaeger.readthedocs.io/en/latest/',
Expand Down Expand Up @@ -111,16 +114,27 @@ describe('<TopNav>', () => {
});

describe('<CustomNavDropdown>', () => {
let subMenu;

beforeEach(() => {
wrapper = shallow(<TopNav.CustomNavDropdown {...configMenuGroup} />);
subMenu = shallow(wrapper.find('Dropdown').props().overlay);
});

it('renders sub-menu text', () => {
dropdownItems.slice(0, 0).forEach(itemConfig => {
const item = subMenu.find(`[text="${itemConfig.label}"]`);
expect(item.length).toBe(1);
expect(item.prop('disabled')).toBe(true);
});
});

it('renders sub-menu items', () => {
const subMenu = shallow(wrapper.find('Dropdown').props().overlay);
dropdownItems.forEach(itemConfig => {
it('renders sub-menu links', () => {
dropdownItems.slice(1, 2).forEach(itemConfig => {
const item = subMenu.find(`[href="${itemConfig.url}"]`);
expect(item.length).toBe(1);
expect(item.prop('target')).toBe(itemConfig.anchorTarget || '_blank');
expect(item.text()).toBe(itemConfig.label);
});
});
});
Expand Down
17 changes: 10 additions & 7 deletions packages/jaeger-ui/src/components/App/TopNav.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,19 +68,22 @@ if (getConfigValue('qualityMetrics.menuEnabled')) {
});
}

function getItemLink(item: ConfigMenuItem) {
function getItem(item: ConfigMenuItem) {
const { label, anchorTarget, url } = item;
const link = (
<a href={url} target={anchorTarget || '_blank'} rel="noopener noreferrer">
{label}
</a>
);
return (
<Menu.Item key={label}>
<a href={url} target={anchorTarget || '_blank'} rel="noopener noreferrer">
{label}
</a>
<Menu.Item key={label} disabled={!url}>
{url ? link : label}
</Menu.Item>
);
}

function CustomNavDropdown({ label, items }: ConfigMenuGroup) {
const menuItems = <Menu>{items.map(getItemLink)}</Menu>;
const menuItems = <Menu>{items.map(getItem)}</Menu>;
return (
<Dropdown overlay={menuItems} placement="bottomRight">
<a>
Expand All @@ -103,7 +106,7 @@ export function TopNavImpl(props: Props) {
<Menu theme="dark" mode="horizontal" selectable={false} className="ub-right" selectedKeys={[pathname]}>
{menuItems.map(m => {
if (isItem(m)) {
return getItemLink(m);
return getItem(m);
}
return (
<Menu.Item key={m.label}>
Expand Down
5 changes: 5 additions & 0 deletions packages/jaeger-ui/src/constants/default-config.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import deepFreeze from 'deep-freeze';

import { FALLBACK_DAG_MAX_NUM_SERVICES } from './index';

const { version } = require('../../package.json');
Copy link
Member

Choose a reason for hiding this comment

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

It might be misleading/confusing to display the UI project version rather than the backend release version (which implies a certain UI version).

Copy link

Choose a reason for hiding this comment

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

+1. We may need to supply release version to the CONFIG here

jaeger-ui/packages/jaeger-ui/public/index.html

Lines 18 to 27 in 2a55237

// Jaeger UI config data is embedded by the query-service. This is
// later merged with defaults into the redux `state.config` via
// src/utils/config/get-config.js. The default provided by the query
// service should be an empty object or it can leave `DEFAULT_CONFIG`
// unchanged.
function getJaegerUiConfig() {
const DEFAULT_CONFIG = null;
const JAEGER_CONFIG = DEFAULT_CONFIG;
return JAEGER_CONFIG;
}

This is replaced in server code with a JSON object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Finally got some time to come back to this -- gave it a shot in jaegertracing/jaeger#2406

Copy link
Member

Choose a reason for hiding this comment

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

alternative solution that does not touch the config: jaegertracing/jaeger#2547

Copy link
Member

Choose a reason for hiding this comment

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

Would you like to give it another try? If you sync to jaeger-ui/master, the index.html now gets the following top level function:

// Jaeger version data is embedded by the query-service via search/replace.
function getJaegerVersion() {
  const DEFAULT_VERSION = {"gitCommit":"", "gitVersion":"", "buildDate":""};
  const JAEGER_VERSION = {"gitCommit":"0f88343172c0bb16ad5c1805d18e20d59176ea7e","gitVersion":"v1.20.0","buildDate":"2020-10-09T17:42:48Z"};
  return JAEGER_VERSION;
}

Considering that there are several data elements here, I would recommend adding them at the bottom of the About menu, e.g.

  • Jaeger v1.20.0
  • Commit 2a55237
  • Build 2020-10-09T17:42:48Z
  • Jaeger UI v1.9.0

Copy link
Contributor Author

@alanisaac alanisaac Oct 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Made the updates as requested, though this does make an assumption about the correct short hash for git being 7 characters. While this is the default, my understanding is that's not always the case if there are collisions and ambiguity.

I think the more technically correct way to get the short hash might be to use git rev-parse --short <SHA> at the source and inject it instead, though this would involve more changes on the Jaeger query side.

image

On a side note, in testing with Jaeger, I found that the default recommended way to run of make run-all-in-one does not include the $(BUILD_INFO) necessary to inject the version information, so the query version info in the about menu appears blank when running via this method. Is this desired or not? For convenience in my own local testing I temporarily added $(BUILD_INFO) to the command.

Copy link
Member

Choose a reason for hiding this comment

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

Looks awesome. One other idea I had is to make versions actually clickable, to that vN.N.N links would take you to the respective release notes page and the commit hash to that commit (the latter is less important than the versions). But this can be done in a separate PR, I would like to merge this first.

Regarding make run-all-in-one, sounds like something we could improve in the make target, although when running from a local copy getting the versions is a bit fuzzy.


export default deepFreeze(
Object.defineProperty(
{
Expand All @@ -33,6 +35,9 @@ export default deepFreeze(
{
label: 'About Jaeger',
items: [
{
label: `Jaeger UI v${version}`,
},
{
label: 'Website/Docs',
url: 'https://www.jaegertracing.io/',
Expand Down