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

[Blocked] Query-Service config and runtime configuration of URL path-prefix #151

Closed
wants to merge 7 commits into from
Closed

[Blocked] Query-Service config and runtime configuration of URL path-prefix #151

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
09d91e6
Add query-service config for URL path prefix
tiffon Dec 21, 2017
f397a31
Comment fix in index.html
tiffon Dec 21, 2017
26f3fd9
Disable code-splitting via react-app-rewired
tiffon Dec 23, 2017
7779eb5
Add react-app-rewired to package.json, fix svg ref
tiffon Dec 23, 2017
f7fef18
Rename placeholder config vars in index.html
tiffon Dec 24, 2017
8bccc2f
Merge branch 'master' into issue-42-query-service-config
tiffon Feb 4, 2018
cddac1b
query-service to set path-prefix on static assets
tiffon Feb 6, 2018
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
30 changes: 30 additions & 0 deletions config-overrides.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// Copyright (c) 2017 Uber Technologies, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

// eslint-disable-next-line import/no-extraneous-dependencies
const webpack = require('webpack');

module.exports = function override(config, env) {
if (env === 'production') {
config.plugins.push(
// prevent code-splitting to allow the URL path-prefix for the site to be
// configurable at runtime
// https://github.com/jaegertracing/jaeger-ui/issues/42
new webpack.optimize.LimitChunkCountPlugin({
Copy link
Member

Choose a reason for hiding this comment

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

so which way are we going, global search&replace of the path in the query service or using this plugin to not split the js files?

Copy link
Member Author

Choose a reason for hiding this comment

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

@yurishkuro Still up for debate.

The main considerations:

  • Primarily - The exact form the loading of JS chunks takes (which is the site of the search/replace) has an unknown level of risk of changing
  • The search/replace in the JS files is not as "clean" as the others
  • The main.js filesize is ~2M, on disk (not sure to what extent that is much of a consideration)

Note: every *.js file, not just main.NNNN.js, must be checked for the static/js path and prefixed if found

So, my main concern is around maintainability.

The above inconveniences allow us to avert a likely hit on initial page-load time, possibly a ~5% increase.

The ideal scenario, IMO, is to do the global replace and if maintaining it is a PITA then revert and go with the plugin approach to not split the files. Not sure how feasible that approach is on the query-service side.

Would that work for you?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I would suggest trying global replace first.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good. This PR needs some rework, then.

maxChunks: 1,
})
);
}
return config;
};
21 changes: 11 additions & 10 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@
"license": "MIT",
"proxy": {
"/api": {
"target": "http://localhost:16686",
"logLevel": "silent",
"secure": false,
"changeOrigin": true,
"ws": true,
"xfwd": true
"target": "http://localhost:16686",
"logLevel": "silent",
"secure": false,
"changeOrigin": true,
"ws": true,
"xfwd": true
}
},
"homepage": null,
Expand All @@ -31,6 +31,7 @@
"husky": "^0.14.3",
"lint-staged": "^4.0.3",
"prettier": "^1.5.3",
"react-app-rewired": "^1.4.0",
"react-scripts": "^1.0.11",
"react-test-renderer": "^15.6.1",
"sinon": "^3.2.1"
Expand Down Expand Up @@ -82,12 +83,12 @@
"tween-functions": "^1.2.0"
},
"scripts": {
"start": "react-scripts start",
"start": "react-app-rewired start",
"start:docs": "REACT_APP_DEMO=true react-scripts start",
"build": "react-scripts build",
"build": "react-app-rewired build",
"eject": "react-scripts eject",
"test": "CI=1 react-scripts test --env=jsdom --color",
"test-dev": "react-scripts test --env=jsdom",
"test": "CI=1 react-app-rewired test --env=jsdom --color",
"test-dev": "react-app-rewired test --env=jsdom",
"coverage": "npm run test -- --coverage",
"lint": "npm run eslint && npm run prettier && npm run flow && npm run check-license",
"eslint": "eslint src",
Expand Down
21 changes: 16 additions & 5 deletions public/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,24 @@
<script>
// 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`
// src/utils/config#getUiConfig. The default provided by the query
// service should be an empty object or it can leave `DEFAULT_UI_CONFIG`
// unchanged.
function getJaegerUiConfig() {
const DEFAULT_CONFIG = null;
const JAEGER_CONFIG = DEFAULT_CONFIG;
return JAEGER_CONFIG;
const DEFAULT_UI_CONFIG = null;
const JAEGER_UI_CONFIG = DEFAULT_UI_CONFIG;
Copy link
Member

Choose a reason for hiding this comment

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

why do we need DEFAULT_ constants? When I originally proposed them I thought they would have some concrete values, and it was easier to search&replace the string JAEGER_UI_CONFIG = DEFAULT_UI_CONFIG than to replace the JS expression on the right hand side. But these are always null anyway, we might as well have JAEGER_UI_CONFIG = null, it's just as easy to search&replace.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could do that.

I think tweaking the name a bit to imply it might be over-written has some value, though:

// use `null` as a fallback value if the query-service does not embed a value
const __INJECTED_UI_CONFIG__ = null;
const JAEGER_UI_CONFIG = __INJECTED_UI_CONFIG__;

Copy link
Member

Choose a reason for hiding this comment

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

+1 for __INJECTED_UI_CONFIG__. I don't think you need the second const, you can simply return __INJECTED_UI_CONFIG__;.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great. Done.

return JAEGER_UI_CONFIG;
}

// Query-service config data is embedded by the query-service. This is
// later merged with defaults into the redux `state.config.queryService`
// via src/utils/config#getQuerySvcConfig. The default provided by the
// query service should be an empty object or it can leave
// `DEFAULT_QS_CONFIG` unchanged.
function getJaegerQuerySvcConfig() {
const DEFAULT_QS_CONFIG = null;
const JAEGER_QS_CONFIG = DEFAULT_QS_CONFIG;
return JAEGER_QS_CONFIG;
}
</script>
</head>
Expand Down
3 changes: 2 additions & 1 deletion src/components/App/Page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
// limitations under the License.

import * as React from 'react';
import _get from 'lodash/get';
import Helmet from 'react-helmet';
import { connect } from 'react-redux';
import type { Location } from 'react-router-dom';
Expand Down Expand Up @@ -51,7 +52,7 @@ export class PageImpl extends React.Component<PageProps> {

render() {
const { children, config } = this.props;
const menu = config && config.menu;
const menu = _get(config, 'ui.menu');
return (
<section className="jaeger-ui-page" id="jaeger-ui">
<Helmet title="Jaeger UI" />
Expand Down
4 changes: 2 additions & 2 deletions src/components/App/TopNav.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import { Dropdown, Menu } from 'semantic-ui-react';

import TraceIDSearchInput from './TraceIDSearchInput';
import type { ConfigMenuItem, ConfigMenuGroup } from '../../types/config';
import getConfig from '../../utils/config/get-config';
import { getUiConfig } from '../../utils/config';
import prefixUrl from '../../utils/prefix-url';

import './TopNav.css';
Expand Down Expand Up @@ -65,7 +65,7 @@ const NAV_LINKS = [
},
];

if (_get(getConfig(), 'dependencies.menuEnabled')) {
if (_get(getUiConfig(), 'dependencies.menuEnabled')) {
NAV_LINKS.push({
key: 'dependencies',
to: prefixUrl('/dependencies'),
Expand Down
4 changes: 2 additions & 2 deletions src/components/DependencyGraph/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import NotFound from '../App/NotFound';
import * as jaegerApiActions from '../../actions/jaeger-api';
import { nodesPropTypes, linksPropTypes } from '../../propTypes/dependencies';
import { formatDependenciesAsNodesAndLinks } from '../../selectors/dependencies';
import getConfig from '../../utils/config/get-config';
import { getUiConfig } from '../../utils/config';
import { FALLBACK_DAG_MAX_NUM_SERVICES } from '../../constants';

import './DependencyGraph.css';
Expand All @@ -37,7 +37,7 @@ export const GRAPH_TYPES = {
};

const dagMaxNumServices =
_get(getConfig(), 'dependencies.dagMaxNumServices') || FALLBACK_DAG_MAX_NUM_SERVICES;
_get(getUiConfig(), 'dependencies.dagMaxNumServices') || FALLBACK_DAG_MAX_NUM_SERVICES;

export default class DependencyGraphPage extends Component {
static propTypes = {
Expand Down
2 changes: 1 addition & 1 deletion src/components/SearchTracePage/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ export default class SearchTracePage extends Component {
!hasTraceResults && (
<div className="ui middle aligned center aligned grid" style={{ marginTop: 100 }}>
<div className="column">
<img className="js-test-logo" alt="presentation" src={JaegerLogo} width="400" />
<img className="js-test-logo" alt="presentation" src={prefixUrl(JaegerLogo)} width="400" />
Copy link
Member

Choose a reason for hiding this comment

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

was it using relative url before?

Copy link
Member Author

@tiffon tiffon Dec 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

No, it uses an absolute path.

The image is imported in JS:

jaeger-ui/src/components/SearchTracePage/index.js

Line 25 in 7779eb5

import JaegerLogo from '../../img/jaeger-logo.svg';

That does two things:

  • Causes the asset to be stored in /static/media/ with a filename derived from the file's hash
  • Stores the eventual absolute path of the media asset in the variable JaegerLogo

The absolute path is then used in the JSX as you excerpted.

The prefixUrl(...) adds the path-prefix, if it is configured.

</div>
</div>
)}
Expand Down
21 changes: 21 additions & 0 deletions src/constants/default-query-svc-config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Copyright (c) 2017 Uber Technologies, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

import deepFreeze from 'deep-freeze';

export default deepFreeze({
pathPrefix: null,
});

export const deprecations = [];
0 src/constants/default-config.js → src/constants/default-ui-config.js
View file
Open in desktop
File renamed without changes.
7 changes: 5 additions & 2 deletions src/reducers/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,14 @@
// See the License for the specific language governing permissions and
// limitations under the License.

import getConfig from '../utils/config/get-config';
import { getQuerySvcConfig, getUiConfig } from '../utils/config';

export default function reduceConfig(state) {
if (state === undefined) {
return getConfig();
return {
queryService: getQuerySvcConfig(),
ui: getUiConfig(),
};
}
return state;
}
44 changes: 0 additions & 44 deletions src/utils/config/get-config.js
View file
Open in desktop

This file was deleted.

87 changes: 0 additions & 87 deletions src/utils/config/get-config.test.js
View file
Open in desktop

This file was deleted.

Loading