Change UI configuration to a Javascript file

[yurishkuro]

@tiffon you proposed changing the UI config file from a JSON file to a Javascript file, to allow comments, etc. Does it require any changes in the index.html?

[tiffon]

@yurishkuro No, it does not. It will require additional guidance, though, as not any valid JavaScript file would be valid.

For instance, these two code blocks would be valid JavaScript for the UI config:

{
  // this is some setting
  someSetting: 'abc'
}

Or

(function() {
  const regex = /abc/i;
  function someUtil(str) {
    return regex.test(str);
  }
  return {
    someSetting: someUtil
  };
})();

On the other hand, this file would be invalid:

const regex = /abc/i;
function someUtil(str) {
  return regex.test(str);
}
const config = {
  someSetting: someUtil
};

The guidance for this is simple, it should either be an object or an IIFE (without a preceding semicolin) that returns an object.

[yurishkuro]

why not give the instructions that the JS file should have the following statement at the end

JAEGER_CONFIG = {
    // config here
};

[tiffon]

That works, too.

It should have const:

const JAEGER_CONFIG = {
  // config here
};

In which case, from the index.html file:

    <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`
      // unchanged.
      function getJaegerUiConfig() {
        const DEFAULT_CONFIG = null;
        const JAEGER_CONFIG = DEFAULT_CONFIG;
        return JAEGER_CONFIG;
      }
    </script>
  </head>

The following should be wholly replaced by the contents of the UI config JavaScript file:

const JAEGER_CONFIG = DEFAULT_CONFIG;

[tiffon]

@yurishkuro The above would not require any code change to the UI repo.

[tiffon]

@yurishkuro One downside of requiring const JAEGER_CONFIG = is JSON will no longer be valid.

[yurishkuro]

why not? If the input file is JSON it will replace the DEFAULT_CONFIG on the right hand side of the assignment. If the input is JS then it will replace the whole statement.

[th3M1ke]

@yurishkuro is this still relevant? Can I take it?

[yurishkuro]

@th3M1ke it's relevant, go for it. Before doing an actual coding I would advise to post some form of design showing how this will work exactly, i.e. what index.html would look like after this change. Note that this will require changes in the Go backend.

[th3M1ke]

Hey, @yurishkuro!
My suggestion for a possible solution is next:

Jaeger UI changes:

1. JS file format:

function loadJsConfig() {
  // load some action once on init
  return {
    prop1: () => {},
    prop2: () => {},
  }
}

2. It will be good to leave for users the possibility to add JSON config as well, but JS config should override JSON config, so priority will be(from lowest to highest): default config -> JSON config -> JS config
3. To achieve the possibility of having both files, we have to add a function in the window scope for JS config injection, like for JSON is(getJaegerUiConfig inside index.html)

function getJaegerUiConfigJS() {
    const DEFAULT_CONFIG_JS = null;
    const JAEGER_JS_CONFIG = DEFAULT_CONFIG_JS;
    return JAEGER_JS_CONFIG;
}

4. Follow the same approach as getJaegerUiConfig we add our function to getConfig.tsx

let cachedJSConfig = null;
export default function getConfig() {
    // ...some code above
    const getJaegerJSConfig = window.getJaegerUiJsConfig;
    if (typeof getJaegerJSConfig !== 'function') {
        if (!haveWarnedFactoryFnJs) {
            // eslint-disable-next-line no-console
            console.warn('Embedded config not available');
            haveWarnedFactoryFnJs = true;
        }
    }
    if (haveWarnedFactoryFnJson && haveWarnedFactoryFnJs) {
        return {
            ...defaultConfig
        };
    }
    const jsonConfig = haveWarnedFactoryFnJson ? null : getJaegerUiConfig();
    let jsConfig = cachedJSConfig;
    if (!haveWarnedFactoryFnJs && !cachedJSConfig) {
        jsConfig = getJaegerJSConfig();
        cachedJSConfig = jsConfig;
    }
    const embedded = {
        ...jsonConfig,
        ...jsConfig
    };
    // ...some code bellow
}

5. Because getConfig widely use and we need to execute loadJsConfig only once and then return only the object, we need to cache it. like following the haveWarnedFactoryFn approach we have to create cachedJSConfig variable
6. We can access JS file config functions via getConfig().prop1()

Jaeger Query

1 Add a flag --query.js-config, which will be the path to the JS file
2. Create a pattern for replacement
jsConfigPattern = regexp.MustCompile("JAEGER_JS_CONFIG *= *DEFAULT_JS_CONFIG;")
3. Create a function to load JS file:

func loadJSConfig(jsConfig string)([] byte, error) {​
    if jsConfig == "" {​
        return nil, nil​
    }​
    bytes, err: = ioutil.ReadFile(filepath.Clean(jsConfig))​ if err != nil {​
        return nil, fmt.Errorf("cannot read Javascript config file %v: %w", jsConfig, err)​
    }​
    return bytes, nil
}

We cannot use loadUIConfig because it has different return types: []byte and map[string]interface{}
4. Replace jsConfigPattern inside index.html to JS file provided with --query.js-config flag inside loadAndEnrichIndexHTML function inside cmd/query/app/static_handler.go

As a usage example, we can use JS config files for customization or adding extra functionality like for this issue: #652

Also, we can use some async calls inside JS config functions to retrieve specific configuration from the server

[yurishkuro]

Hi @th3M1ke ,

A few suggestions:

1. The JS file format looks good. We'll probably need to brainstorm in the future how to define its data schema, e.g. the long term goal is to be able to load extensions into the UI using this file.
2. I would suggest avoiding creating new top-level functions. The rest of the UI code does not need to know how the config was initialized, it should continue calling getJaegerUiConfig, while the difference between JSON or JS should be sorted out locally inside that function.
3. We certainly do want to continue supporting plain JSON, but I don't think we need to support both at the same time, i.e. the user can only have one or the other.
4. I don't think we need a new CLI option for the query service - the existing option should simply recognize the extension of the file and do the right thing.

[th3M1ke]

@yurishkuro Have raised a PR(jaegertracing/jaeger#2707) in jaeger-query repo. Currently, no changes form the UI side required

[yurishkuro]

this is not resolved until backend PR (jaegertracing/jaeger#2707) is merged.