-
Notifications
You must be signed in to change notification settings - Fork 483
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
Added jaeger ui version to about menu #606
Conversation
Signed-off-by: Alan Pinkert <[email protected]>
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
@@ -16,6 +16,8 @@ import deepFreeze from 'deep-freeze'; | |||
|
|||
import { FALLBACK_DAG_MAX_NUM_SERVICES } from './index'; | |||
|
|||
const { version } = require('../../package.json'); |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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 <[email protected]>
Signed-off-by: Alan Pinkert <[email protected]>
Signed-off-by: Alan Pinkert <[email protected]>
Signed-off-by: Alan Pinkert <[email protected]>
Signed-off-by: Alan Pinkert <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot!
* Added jaeger ui version to about menu Signed-off-by: Alan Pinkert <[email protected]> * Added Jaeger version info from config Signed-off-by: Alan Pinkert <[email protected]> * Removed extra 'v' from Jaeger version Signed-off-by: Alan Pinkert <[email protected]> * Changed to use short git commit hash Signed-off-by: Alan Pinkert <[email protected]> * Added test for missing coverage Signed-off-by: Alan Pinkert <[email protected]> Signed-off-by: vvvprabhakar <[email protected]>
* Added jaeger ui version to about menu Signed-off-by: Alan Pinkert <[email protected]> * Added Jaeger version info from config Signed-off-by: Alan Pinkert <[email protected]> * Removed extra 'v' from Jaeger version Signed-off-by: Alan Pinkert <[email protected]> * Changed to use short git commit hash Signed-off-by: Alan Pinkert <[email protected]> * Added test for missing coverage Signed-off-by: Alan Pinkert <[email protected]> Signed-off-by: vvvprabhakar <[email protected]>
* Added jaeger ui version to about menu Signed-off-by: Alan Pinkert <[email protected]> * Added Jaeger version info from config Signed-off-by: Alan Pinkert <[email protected]> * Removed extra 'v' from Jaeger version Signed-off-by: Alan Pinkert <[email protected]> * Changed to use short git commit hash Signed-off-by: Alan Pinkert <[email protected]> * Added test for missing coverage Signed-off-by: Alan Pinkert <[email protected]> Signed-off-by: vvvprabhakar <[email protected]>
Signed-off-by: Alan Pinkert [email protected]
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.