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

tiffon · 2017-12-23T08:19:15Z

This PR is no longer relevant.

Refer to #198.

Fix #42.

Adds config data from the query service that is separate from the UI config, dubbed querySvcConfig.

Presently, this contains a single property, pathPrefix, which causes the UI to handle being served from a URL prefix, at runtime.

When set, the pathPrefix should be formatted as /xyz.

Query-service changes

Changes in the query-service this PR relies on:

Config injection

Related to the UI config regex:
- Rename DEFAULT_CONFIG to __INJECTED_UI_CONFIG__
- Change the regex to now handle the return __INJECTED_UI_CONFIG__; replacement
Treatment similar to the UI config for the query-service config
- The variable name is: __INJECTED_QS_CONFIG__
If the URL path-prefix is set, it needs to be passed into index.html in a manner similar to the UI config, e.g. as JSON:
- {"pathPrefix":"/xyz"}

Static assets that need URL rewrites in their content

In order for the URL path prefix to work, the all URLs need to have the prefix pre-pended. This essentially amounts to the reference to the favicon and references to static assets (including references in static assets).

Signed-off-by: Joe Farro <[email protected]>

yurishkuro · 2017-12-23T16:33:55Z

Incidentally, the URLs are not constants but instead are derived from expressions. Therefore, while possible, rewriting the static asset URLs in the JS files looks precarious and error-prone.

wouldn't they still have a fixed prefix /static/js that is easy to search & replace? Doesn't seem that much different from how the query service would have to pre-process the other files.

yurishkuro · 2017-12-23T16:38:31Z

public/index.html

-        const JAEGER_CONFIG = DEFAULT_CONFIG;
-        return JAEGER_CONFIG;
+        const DEFAULT_UI_CONFIG = null;
+        const JAEGER_UI_CONFIG = DEFAULT_UI_CONFIG;


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.

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__;

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

Great. Done.

yurishkuro · 2017-12-23T16:40:16Z

src/components/SearchTracePage/index.js

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


was it using relative url before?

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.

tiffon · 2017-12-23T22:43:52Z

@yurishkuro

wouldn't they still have a fixed prefix /static/js that is easy to search & replace? Doesn't seem that much different from how the query service would have to pre-process the other files.

It's not that much different, and we can go down that road. It would spare any impact to the initial page-load time and remove the need for react-app-rewired. The expression(s) in main.NNNN.js that would need to be updated are generally of the form:

u.src=t.p+"static/js/"+({}[e]||e)+"."+{0:"03cf6b4e"}[e]+".chunk.js";

The filesize of main...js is around 2 meg.

Signed-off-by: Joe Farro <[email protected]>

yurishkuro · 2017-12-24T22:32:08Z

config-overrides.js

+      // 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({


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?

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

Yes, I would suggest trying global replace first.

Sounds good. This PR needs some rework, then.

tiffon · 2018-01-03T16:11:48Z

Closing, will re-open when I've reverted the chunkless changes.

Signed-off-by: Joe Farro <[email protected]>

tiffon · 2018-02-06T15:45:00Z

Reopening this PR. Main change necessary, from the previous state, was to revert the removal of the chunk generation.

It's worth noting, this PR requires several changes to the query-service, as noted in the PR description.

codecov · 2018-02-06T15:53:56Z

Codecov Report

Merging #151 into master will increase coverage by 0.02%.
The diff coverage is 95.83%.

@@            Coverage Diff            @@
##           master    #151      +/-   ##
=========================================
+ Coverage   90.77%   90.8%   +0.02%     
=========================================
  Files          93      94       +1     
  Lines        1930    1935       +5     
  Branches      383     382       -1     
=========================================
+ Hits         1752    1757       +5     
  Misses        155     155              
  Partials       23      23

Impacted Files	Coverage Δ
src/constants/default-ui-config.js	`100% <ø> (ø)`
src/utils/metrics.js	`0% <0%> (ø)`	⬆️
src/reducers/config.js	`66.66% <100%> (ø)`	⬆️
src/constants/default-query-svc-config.js	`100% <100%> (ø)`
src/components/DependencyGraph/index.js	`100% <100%> (ø)`	⬆️
src/components/App/TopNav.js	`75% <100%> (ø)`	⬆️
src/utils/config/index.js	`100% <100%> (ø)`
src/utils/prefix-url.js	`100% <100%> (ø)`	⬆️
src/components/App/Page.js	`100% <100%> (ø)`	⬆️
... and 1 more

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 b7a3e74...cddac1b. Read the comment docs.

saminzadeh · 2018-02-06T16:04:34Z

Looking back at this, create react app documents a section on this.

Looks like we are doing this already +1

tiffon

@ys can you mark this PR with "Request changes" so merging it will be blocked until the necessary changes in the query-service are ready?

black-adder

I don't like this one bit, scrap it all

ys · 2018-02-12T20:50:19Z

Can you please stop mentioning me all the time here! I think it is about someone else

tiffon · 2018-02-12T20:58:07Z

@ys Apologies! I'll block you so it doesn't happen again!

tiffon · 2018-03-02T04:32:06Z

@yurishkuro Doesn't look like the work needed in the query-service for this PR is going to be on the docket anytime soon... Think it makes sense to close this PR?

tiffon · 2018-03-13T17:32:13Z

This PR is waiting on work from the query-service, which is currently not scheduled. So, I'm closing and will mark the ticket as blocked.

yurishkuro · 2018-03-13T17:34:24Z

This PR is waiting on work from the query-service, which is currently not scheduled.

Do we have a ticket in the main repo w/ description of what needs to be done? We can tag it "help wanted".

tiffon · 2018-03-18T21:09:45Z

@yurishkuro I'll create an issue in the main repo once I create the PR for the <base> approach.

tiffon added 3 commits December 21, 2017 16:29

Add query-service config for URL path prefix

09d91e6

Signed-off-by: Joe Farro <[email protected]>

Comment fix in index.html

f397a31

Signed-off-by: Joe Farro <[email protected]>

Disable code-splitting via react-app-rewired

26f3fd9

Signed-off-by: Joe Farro <[email protected]>

tiffon requested review from black-adder, vprithvi, yurishkuro and saminzadeh December 23, 2017 08:19

Add react-app-rewired to package.json, fix svg ref

7779eb5

Signed-off-by: Joe Farro <[email protected]>

yurishkuro reviewed Dec 23, 2017

View reviewed changes

yurishkuro mentioned this pull request Dec 23, 2017

Add View Option for raw/unadjusted trace #153

Merged

Rename placeholder config vars in index.html

f7fef18

Signed-off-by: Joe Farro <[email protected]>

ghost assigned tiffon Dec 24, 2017

ghost added the review label Dec 24, 2017

yurishkuro reviewed Dec 24, 2017

View reviewed changes

yurishkuro approved these changes Dec 24, 2017

View reviewed changes

tiffon closed this Jan 3, 2018

ghost removed the review label Jan 3, 2018

tiffon added 2 commits February 4, 2018 12:51

Merge branch 'master' into issue-42-query-service-config

8bccc2f

Signed-off-by: Joe Farro <[email protected]>

query-service to set path-prefix on static assets

cddac1b

Signed-off-by: Joe Farro <[email protected]>

tiffon reopened this Feb 6, 2018

ghost added the review label Feb 6, 2018

simonrobb approved these changes Feb 6, 2018

View reviewed changes

tiffon commented Feb 10, 2018

View reviewed changes

black-adder suggested changes Feb 12, 2018

View reviewed changes

tiffon changed the title ~~Query-Service config and runtime configuration of URL path-prefix~~ [Blocked] Query-Service config and runtime configuration of URL path-prefix Feb 13, 2018

tiffon closed this Mar 13, 2018

ghost removed the review label Mar 13, 2018

tiffon added query-service blocked labels Mar 13, 2018

tiffon mentioned this pull request Mar 13, 2018

No support for Jaeger behind a reverse proxy #42

Closed

yurishkuro deleted the issue-42-query-service-config branch January 29, 2020 15:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tiffon commented Dec 23, 2017 •

edited

Loading

yurishkuro commented Dec 23, 2017

yurishkuro Dec 23, 2017

tiffon Dec 23, 2017

yurishkuro Dec 24, 2017

tiffon Dec 24, 2017

yurishkuro Dec 23, 2017

tiffon Dec 23, 2017 •

edited

Loading

tiffon commented Dec 23, 2017 •

edited

Loading

yurishkuro Dec 24, 2017

tiffon Dec 25, 2017

yurishkuro Dec 25, 2017

tiffon Dec 25, 2017

tiffon commented Jan 3, 2018

tiffon commented Feb 6, 2018

codecov bot commented Feb 6, 2018 •

edited

Loading

saminzadeh commented Feb 6, 2018

tiffon left a comment

black-adder left a comment

ys commented Feb 12, 2018

tiffon commented Feb 12, 2018

tiffon commented Mar 2, 2018

tiffon commented Mar 13, 2018

yurishkuro commented Mar 13, 2018

tiffon commented Mar 18, 2018

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

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

Conversation

tiffon commented Dec 23, 2017 • edited Loading

This PR is no longer relevant.

Query-service changes

Config injection

Static assets that need URL rewrites in their content

yurishkuro commented Dec 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiffon Dec 23, 2017 • edited Loading

Choose a reason for hiding this comment

tiffon commented Dec 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiffon commented Jan 3, 2018

tiffon commented Feb 6, 2018

codecov bot commented Feb 6, 2018 • edited Loading

Codecov Report

saminzadeh commented Feb 6, 2018

tiffon left a comment

Choose a reason for hiding this comment

black-adder left a comment

Choose a reason for hiding this comment

ys commented Feb 12, 2018

tiffon commented Feb 12, 2018

tiffon commented Mar 2, 2018

tiffon commented Mar 13, 2018

yurishkuro commented Mar 13, 2018

tiffon commented Mar 18, 2018

tiffon commented Dec 23, 2017 •

edited

Loading

tiffon Dec 23, 2017 •

edited

Loading

tiffon commented Dec 23, 2017 •

edited

Loading

codecov bot commented Feb 6, 2018 •

edited

Loading