Add ability to use JS file for UI configuration (#123 from jaeger-ui) #2707

th3M1ke · 2020-12-29T14:45:16Z

Which problem is this PR solving?

resolves Change UI configuration to a Javascript file jaeger-ui#123

Short description of the changes

Added ability to use javascript file for UI configuration

…om jaeger-ui) Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io>

codecov · 2020-12-29T16:02:22Z

Codecov Report

Merging #2707 (12394b3) into master (2f8ffa9) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2707      +/-   ##
==========================================
+ Coverage   95.73%   95.77%   +0.03%     
==========================================
  Files         216      217       +1     
  Lines        9599     9628      +29     
==========================================
+ Hits         9190     9221      +31     
+ Misses        336      335       -1     
+ Partials       73       72       -1

Impacted Files	Coverage Δ
cmd/query/app/static_handler.go	`96.77% <100.00%> (+1.99%)`	⬆️
pkg/discovery/grpcresolver/grpc_resolver.go	`100.00% <0.00%> (ø)`
cmd/agent/app/reporter/connect_metrics.go	`100.00% <0.00%> (ø)`
cmd/agent/app/reporter/grpc/builder.go	`95.83% <0.00%> (+0.83%)`	⬆️

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 dc091c5...12394b3. Read the comment docs.

yurishkuro

Good start!

yurishkuro · 2020-12-29T16:02:53Z

cmd/query/app/static_handler.go

-		bytes, _ := json.Marshal(config)
-		configString = fmt.Sprintf("JAEGER_CONFIG = %v", string(bytes))
+	} else if configBytes != nil {
+		configString = fmt.Sprintf("JAEGER_CONFIG = %v", string(configBytes))


This doesn't look right. In case of JS file that contains a function there needs to be a part of the template that invokes that function and assigns its result to JAEGER_CONFIG ( or something similar).

I would suggest taking a look at this line from the perspective of invoking the JS config file itself.

My idea is next:

we add a more strict JS file format validation like /^function\s[a-zA-Z$_][a-zA-Z0-9$_]+(\s)?(\s)?{(.*\n)+}$/gi - in this case we can rely on JS file and assume, that its plain JS function like

function LoadCodnfig(){ console.log('Hello World!'); return { callMe: () => { return Promise.resolve(setTimeout(() => { console.log('CALL ME!!!', 1000); })) }, asd: 'sdf' } }

next, we add () at the moment of injecting config inside index.html and we have IIFE inside index.html

in jaeger-ui we will add a cache for getConfig() like

diff --git a/packages/jaeger-ui/src/utils/config/get-config.tsx b/packages/jaeger-ui/src/utils/config/get-config.tsx index 458ba0c..68dfd47 100644 --- a/packages/jaeger-ui/src/utils/config/get-config.tsx +++ b/packages/jaeger-ui/src/utils/config/get-config.tsx @@ -16,15 +16,21 @@ import _get from 'lodash/get'; import processDeprecation from './process-deprecation'; import defaultConfig, { deprecations } from '../../constants/default-config'; +import { Config } from '../../types/config'; let haveWarnedFactoryFn = false; let haveWarnedDeprecations = false; +let cachedConfig: null | Config = null; /** * Merge the embedded config from the query service (if present) with the * default config from `../../constants/default-config`. */ export default function getConfig() { + if(cachedConfig) { + return cachedConfig; + } + const getJaegerUiConfig = window.getJaegerUiConfig; if (typeof getJaegerUiConfig !== 'function') { if (!haveWarnedFactoryFn) { @@ -52,6 +58,7 @@ export default function getConfig() { rv[key] = { ...defaultConfig[key], ...embedded[key] }; } } + cachedConfig = rv; return rv; }

From a usage perspective, we can say, that JS config file is Function and we invoke it once the page loaded

Could you show how index.html would be changed to handle JS file? Right now it looks like this, and the middle line is being replaced with the content of JSON file.

function getJaegerUiConfig() { const DEFAULT_CONFIG = null; const JAEGER_CONFIG = DEFAULT_CONFIG; return JAEGER_CONFIG; }

For JSON:

function getJaegerUiConfig() { const DEFAULT_CONFIG = null; const JAEGER_CONFIG = {"a":1, "b":2}; return JAEGER_CONFIG; }

For JS:

function getJaegerUiConfig() { const DEFAULT_CONFIG = null; const JAEGER_CONFIG = function UIConfig() { return {"a":1, "b":2}; }(); return JAEGER_CONFIG; }

I see, you're suggesting to piggy back on the string replacement that currently happens for const JAEGER_CONFIG = DEFAULT_CONFIG; line. While this is doable, I wonder if it puts a lot of limitations on what the actual config function can do. If all it does is return a static dictionary, then sure, but we may want to use it as a mechanism for loading plugins, etc. Wouldn't it be better if it was embedded at the top level, not inlined inside an expression?

What if we do this instead:

// JAEGER_CONFIG_JS // the line above may be replaced by user-provided JS file that should define a UIConfig function. function getJaegerUiConfig() { if "UIConfig is not null and is a function" { return UIConfig(); } const DEFAULT_CONFIG = null; const JAEGER_CONFIG = DEFAULT_CONFIG; return JAEGER_CONFIG; }

regarding this:

let cachedConfig: null | Config = null;

I think there is a memoize module that we're using, could it be used instead of explicit caching?

Do I understand you correctly?

JSON:

// JAEGER_CONFIG_JS // the line above may be replaced by user-provided JS file that should define a UIConfig function. function getJaegerUiConfig() { if(typeof window.UIConfig === 'function') { return UIConfig(); } const DEFAULT_CONFIG = null; const JAEGER_CONFIG = {a:1, b:2}; return JAEGER_CONFIG; }

JS:

function UIConfig() { return {a:1, b:2} } // the line above may be replaced by user-provided JS file that should define a UIConfig function. function getJaegerUiConfig() { if(typeof window.UIConfig === 'function') { return UIConfig(); } const DEFAULT_CONFIG = null; const JAEGER_CONFIG = {a:1, b:2}; return JAEGER_CONFIG; }

lgtm. any concerns?

No concerns. Thanks for clarifying! 🤝 WIll update PR shortly

bonus points if we can get rid of the comment during substitution (e.g. put it all on one line)

yurishkuro · 2020-12-29T16:06:28Z

cmd/query/app/static_handler.go

 	var unmarshal func([]byte, interface{}) error

 	switch strings.ToLower(ext) {
 	case ".json":
 		unmarshal = json.Unmarshal
+	case ".js":
+		r = bytes.TrimSpace(bytesConfig)
+		if !bytes.HasPrefix(r, []byte("function")) {


I would suggest to check for string "function UIConfig()" if you want some kind of validation, not the prefix, because there may be comments etc.

Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io>

th3M1ke · 2021-01-04T18:44:02Z

Have update the PR. @yurishkuro can you take a look?

yurishkuro

lgtm overall, some clean-up comments

yurishkuro · 2021-01-04T19:02:15Z

cmd/query/app/static_handler.go

@@ -39,6 +40,7 @@ var (

 	// The following patterns are searched and replaced in the index.html as a way of customizing the UI.
 	configPattern   = regexp.MustCompile("JAEGER_CONFIG *= *DEFAULT_CONFIG;")
+	configJsPattern = regexp.MustCompile(`(?im)(^\s+)\/\/.*JAEGER_CONFIG_JS.*\n.*`)


Suggested change

configJsPattern = regexp.MustCompile(`(?im)(^\s+)\/\/.*JAEGER_CONFIG_JS.*\n.*`)

configJsPattern = regexp.MustCompile(`(?im)(^\s+)\/\/\s*JAEGER_CONFIG_JS.*\n.*`)

what is the purpose of trailing .*? Also, with the m mode, does .*\n guarantee single-line match?

.* - my bad, you are correct \s+ is best fits.
m is for multiline search. I assume that we must replace 2 lines: line with JAEGER_CONFIG_JS and following comment on the next line

// JAEGER_CONFIG_JS // the line above may be replaced by user-provided JS file that should define a UIConfig function.

yurishkuro · 2021-01-04T19:07:53Z

cmd/query/app/static_handler.go

@@ -72,6 +74,12 @@ type StaticAssetsHandlerOptions struct {
 	Logger       *zap.Logger
 }

+// UIConfigOptions define options for injecting config in index.html in loadAndEnrichIndexHTML
+type UIConfigOptions struct {


"options" typically refers to inputs, this is used as output

does not need to be public

Suggested change

type UIConfigOptions struct {

type loadedConfig struct {

yurishkuro · 2021-01-04T19:12:44Z

cmd/query/app/static_handler.go

-		bytes, _ := json.Marshal(config)
-		configString = fmt.Sprintf("JAEGER_CONFIG = %v", string(bytes))
+	} else if configObject != nil {
+		indexBytes = configObject.regexp.ReplaceAll(indexBytes, append([]byte(`${1}`), configObject.config...))


I am confused by ${1} - is it just repeating whitespace? If we match on // JAEGER... and replace just that, the whitespace becomes irrelevant.

you are correct, it's for white spaces for function UIConfig(). To save origin indent 😶 . Will remove it.

yurishkuro · 2021-01-04T19:14:28Z

cmd/query/app/static_handler.go

+		}
+		// TODO if we want to support other config formats like YAML, we need to normalize `config` to be
+		// suitable for json.Marshal(). For example, YAML parser may return a map that has keys of type
+		// interface{}, and json.Marshal() is unable to serialize it.


I suggest removing this. We don't have any plans to support anything other than JSON and JS. I would move the unmarshal code directly under case ".json": to simplify the logic.

can also return directly from switch branches instead of maintaining global-like r, re, c variables.

yurishkuro · 2021-01-04T19:18:04Z

cmd/query/app/static_handler_test.go

@@ -117,57 +119,67 @@ func TestNewStaticAssetsHandlerErrors(t *testing.T) {

 // This test is potentially intermittent
 func TestHotReloadUIConfigTempFile(t *testing.T) {


Does reload functionality actually depend on the type of file being loaded? It seems it only complicates the test without any real benefits.

Probably no, but to keep consistency I did these changes. Will revert this back

yurishkuro · 2021-01-04T19:20:34Z

cmd/query/app/static_handler_test.go

 		expectedError string
 	}

+	jsonRegExpPattern := regexp.MustCompile("JAEGER_CONFIG *= *DEFAULT_CONFIG;")
+	jsRegExpPattern := regexp.MustCompile(`(?im)(^\s+)\/\/.*JAEGER_CONFIG_JS.*\n.*`)


Could we just use the constants here? Also, do we even need to be comparing which regex is returned? I would rather express the test in terms of what the resulting HTML looks like, not the implementation details of how that HTML is produced.

Could we just use the constants here?

Sure

Also, do we even need to be comparing which regex is returned?

This is unit test, I believe, and LoadUIConfig is return

type UIConfigOptions struct { regexp *regexp.Regexp config []byte }

regexp is part of it...

I would rather express the test in terms of what the resulting HTML looks like, not the implementation details of how that HTML is produced.

Will update TestRegisterStaticHandler to cover JS file config case

Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io>

th3M1ke · 2021-01-05T19:48:10Z

Have updated the PR. @yurishkuro can you take a look, please? 🙏

yurishkuro

lgtm, a couple of minor nits

yurishkuro · 2021-01-11T04:58:32Z

cmd/query/app/static_handler.go

+		}, nil
+	case ".js":
+		r = bytes.TrimSpace(bytesConfig)
+		if !bytes.Contains(r, []byte("function UIConfig(){")) {


Perhaps it's better to use regex here? Especially the part (){, without space before {, is a matter of style/formatting.

yurishkuro · 2021-01-11T05:01:05Z

cmd/query/app/static_handler.go

+	case ".js":
+		r = bytes.TrimSpace(bytesConfig)
+		if !bytes.Contains(r, []byte("function UIConfig(){")) {
+			return nil, fmt.Errorf("wrong JS function format in UI config file format %v", uiConfig)


more self-explanatory error:

Suggested change

return nil, fmt.Errorf("wrong JS function format in UI config file format %v", uiConfig)

return nil, fmt.Errorf("UI config file must define function UIConfig(): %v", uiConfig)

yurishkuro · 2021-01-11T05:03:18Z

cmd/query/app/static_handler.go

@@ -210,30 +211,43 @@ func loadIndexHTML(open func(string) (http.File, error)) ([]byte, error) {
 	return indexBytes, nil
 }

-func loadUIConfig(uiConfig string) (map[string]interface{}, error) {
+func loadUIConfig(uiConfig string) (*loadedConfig, error) {
 	if uiConfig == "" {
 		return nil, nil
 	}
 	ext := filepath.Ext(uiConfig)


for better readability, I would move this next to the switch condition:

ext := filepath.Ext(uiConfig) switch strings.ToLower(ext) {

yurishkuro · 2021-01-11T05:04:05Z

cmd/query/app/static_handler.go

+		return &loadedConfig{
+			regexp: configJsPattern,
+			config: r,
+		}, nil
 	default:
 		return nil, fmt.Errorf("unrecognized UI config file format %v", uiConfig)


Suggested change

return nil, fmt.Errorf("unrecognized UI config file format %v", uiConfig)

return nil, fmt.Errorf("unrecognized UI config file format, expecting .js or .json file: %v", uiConfig)

yurishkuro · 2021-01-11T05:08:48Z

and sorry for delay, did not get the notification of the last change

Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io>

yurishkuro

🎉 🎉 🎉

@th3M1ke this will also require some change in the jaeger-ui, to the html template and the function that retrieves the config. Did you already implement it? It may be worth trying to build query-service with the updated UI as an end-to-end test of JS configs (before we merge this PR, in case of any issues).

…ation

Pull request has been modified.

th3M1ke · 2021-01-13T16:24:09Z

@yurishkuro I have added a PR with required changes in the jaeger-ui repo: jaegertracing/jaeger-ui#677

yurishkuro · 2021-01-14T01:23:02Z

I suggest we bump the UI git submodule to pick up jaegertracing/jaeger-ui#677 (which I just merged). Then once this PR is merged, the latest Docker image for all-in-one can be used to test JS config.

albertteoh · 2021-01-14T02:35:50Z

I suggest we bump the UI git submodule to pick up jaegertracing/jaeger-ui#677 (which I just merged). Then once this PR is merged, the latest Docker image for all-in-one can be used to test JS config.

Would this require a new release tag of Jaeger-UI?

yurishkuro · 2021-01-14T03:28:21Z

No, submodule links by a commit.

Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io>

yurishkuro

Thanks!

…gertracing#2707) * Add ability to use JS file for UI configuration (#123 from jaeger-ui) Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io> * Update UI config file injection Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io> * Update update test, refactor loadUIConfig function Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io> * Update errors text, minor refactor Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io> * Remove debug code Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io> * Update jaeger-ui to latest master with index.html changes Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io>

th3M1ke requested a review from a team as a code owner December 29, 2020 14:45

th3M1ke requested a review from joe-elliott December 29, 2020 14:45

mergify bot assigned jpkrohling Dec 29, 2020

mergify bot requested a review from jpkrohling December 29, 2020 14:45

th3M1ke mentioned this pull request Dec 29, 2020

Change UI configuration to a Javascript file jaegertracing/jaeger-ui#123

Closed

Add ability to use JS file for UI configuration (jaegertracing#123 fr…

67f35b3

…om jaeger-ui) Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io>

th3M1ke force-pushed the Add-ability-to-use-JS-file-for-UI-configuration branch from 85cc44b to 67f35b3 Compare December 29, 2020 15:49

yurishkuro reviewed Dec 29, 2020

View reviewed changes

albertteoh mentioned this pull request Jan 4, 2021

Flaky test: TestHotReloadUIConfigTempFile #2711

Closed

Update UI config file injection

b1ea51d

Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io>

yurishkuro reviewed Jan 4, 2021

View reviewed changes

Update update test, refactor loadUIConfig function

35039ed

Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io>

yurishkuro mentioned this pull request Jan 11, 2021

Support for customizing the display of operation names jaegertracing/jaeger-ui#675

Open

yurishkuro reviewed Jan 11, 2021

View reviewed changes

th3M1ke added 2 commits January 12, 2021 02:40

Update errors text, minor refactor

02a6292

Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io>

Remove debug code

85c1274

Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io>

yurishkuro previously approved these changes Jan 12, 2021

View reviewed changes

Merge branch 'master' into Add-ability-to-use-JS-file-for-UI-configur…

a7565c0

…ation

th3M1ke mentioned this pull request Jan 13, 2021

Change UI configuration to a Javascript file jaegertracing/jaeger-ui#677

Merged

Update jaeger-ui to latest master with index.html changes

12394b3

Signed-off-by: Mykhailo Semenchenko <mykhailo.semenchenko@logz.io>

yurishkuro approved these changes Jan 14, 2021

View reviewed changes

yurishkuro merged commit b73b0c8 into jaegertracing:master Jan 14, 2021

dependabot bot mentioned this pull request Mar 8, 2021

Bump github.com/jaegertracing/jaeger from 1.21.0 to 1.22.0 httpsgithu/opentelemetry-collector#22

Closed

	configJsPattern = regexp.MustCompile(`(?im)(^\s+)\/\/.JAEGER_CONFIG_JS.\n.*`)
	configJsPattern = regexp.MustCompile(`(?im)(^\s+)\/\/\sJAEGER_CONFIG_JS.\n.*`)

		@@ -117,57 +119,67 @@ func TestNewStaticAssetsHandlerErrors(t *testing.T) {

		// This test is potentially intermittent
		func TestHotReloadUIConfigTempFile(t *testing.T) {

	return nil, fmt.Errorf("wrong JS function format in UI config file format %v", uiConfig)
	return nil, fmt.Errorf("UI config file must define function UIConfig(): %v", uiConfig)

	return nil, fmt.Errorf("unrecognized UI config file format %v", uiConfig)
	return nil, fmt.Errorf("unrecognized UI config file format, expecting .js or .json file: %v", uiConfig)

Add ability to use JS file for UI configuration (#123 from jaeger-ui) #2707

Add ability to use JS file for UI configuration (#123 from jaeger-ui) #2707

Conversation

th3M1ke commented Dec 29, 2020 • edited by yurishkuro Loading

Which problem is this PR solving?

Short description of the changes

codecov bot commented Dec 29, 2020 • edited Loading

Codecov Report

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

th3M1ke Dec 29, 2020 • 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

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

yurishkuro Dec 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

th3M1ke commented Jan 4, 2021

yurishkuro left a 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

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

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

th3M1ke commented Jan 5, 2021

yurishkuro left a 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

Choose a reason for hiding this comment

yurishkuro commented Jan 11, 2021

yurishkuro left a comment

Choose a reason for hiding this comment

th3M1ke commented Jan 13, 2021

yurishkuro commented Jan 14, 2021

albertteoh commented Jan 14, 2021

yurishkuro commented Jan 14, 2021

yurishkuro left a comment

Choose a reason for hiding this comment

th3M1ke commented Dec 29, 2020 •

edited by yurishkuro

Loading

codecov bot commented Dec 29, 2020 •

edited

Loading

th3M1ke Dec 29, 2020 •

edited

Loading

yurishkuro Dec 30, 2020 •

edited

Loading