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

Added jaeger ui version to about menu #606

Merged
merged 6 commits into from
Oct 18, 2020

Conversation

alanisaac
Copy link
Contributor

Signed-off-by: Alan Pinkert alan.pinkert@gmail.com

Which problem is this PR solving?

Short description of the changes

Adds the version of Jaeger UI to the about menu as suggested in this comment #365 (comment). There are many suggestions in the issue thread about where to place the version, this is simply a stab at one of them.

image

Signed-off-by: Alan Pinkert <alan.pinkert@gmail.com>
@codecov
Copy link

codecov bot commented Jul 28, 2020

Codecov Report

Merging #606 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #606      +/-   ##
==========================================
+ Coverage   94.20%   94.22%   +0.01%     
==========================================
  Files         227      228       +1     
  Lines        5906     5919      +13     
  Branches     1484     1488       +4     
==========================================
+ Hits         5564     5577      +13     
  Misses        303      303              
  Partials       39       39              
Impacted Files Coverage Δ
packages/jaeger-ui/src/components/App/TopNav.tsx 92.59% <100.00%> (+0.28%) ⬆️
...ackages/jaeger-ui/src/constants/default-config.tsx 100.00% <100.00%> (ø)
...ckages/jaeger-ui/src/utils/version/get-version.tsx 100.00% <100.00%> (ø)

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 9fa9d74...9f5e7fe. Read the comment docs.

@@ -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 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

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.

Signed-off-by: Alan Pinkert <alan.pinkert@gmail.com>
Signed-off-by: Alan Pinkert <alan.pinkert@gmail.com>
Signed-off-by: Alan Pinkert <alan.pinkert@gmail.com>
Signed-off-by: Alan Pinkert <alan.pinkert@gmail.com>
Signed-off-by: Alan Pinkert <alan.pinkert@gmail.com>
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@yurishkuro yurishkuro merged commit 832b7de into jaegertracing:master Oct 18, 2020
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 4, 2021
* Added jaeger ui version to about menu

Signed-off-by: Alan Pinkert <alan.pinkert@gmail.com>

* Added Jaeger version info from config

Signed-off-by: Alan Pinkert <alan.pinkert@gmail.com>

* Removed extra 'v' from Jaeger version

Signed-off-by: Alan Pinkert <alan.pinkert@gmail.com>

* Changed to use short git commit hash

Signed-off-by: Alan Pinkert <alan.pinkert@gmail.com>

* Added test for missing coverage

Signed-off-by: Alan Pinkert <alan.pinkert@gmail.com>
Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 4, 2021
* Added jaeger ui version to about menu

Signed-off-by: Alan Pinkert <alan.pinkert@gmail.com>

* Added Jaeger version info from config

Signed-off-by: Alan Pinkert <alan.pinkert@gmail.com>

* Removed extra 'v' from Jaeger version

Signed-off-by: Alan Pinkert <alan.pinkert@gmail.com>

* Changed to use short git commit hash

Signed-off-by: Alan Pinkert <alan.pinkert@gmail.com>

* Added test for missing coverage

Signed-off-by: Alan Pinkert <alan.pinkert@gmail.com>
Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021
* Added jaeger ui version to about menu

Signed-off-by: Alan Pinkert <alan.pinkert@gmail.com>

* Added Jaeger version info from config

Signed-off-by: Alan Pinkert <alan.pinkert@gmail.com>

* Removed extra 'v' from Jaeger version

Signed-off-by: Alan Pinkert <alan.pinkert@gmail.com>

* Changed to use short git commit hash

Signed-off-by: Alan Pinkert <alan.pinkert@gmail.com>

* Added test for missing coverage

Signed-off-by: Alan Pinkert <alan.pinkert@gmail.com>
Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
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.

3 participants