Support combined config values from multiple sources

[andymc12]

In looking at possible solutions for MP Rest Client issue 73 - propagating HTTP headers, one issue that arose is how to allow different technologies (JWT, Metrics, etc.) to specify which headers they need propagated. MP Config seems like the most natural fit, but we would need to ability to read the specified config from multiple sources.

For example, JWT might specify that they need headers "A", "B" and "C" propagated, but Metrics might need "X", "Y", and "Z". In order for this to work the MP Rest Client implementation will need the combined list of headers A,B,C,X,Y,Z.

Here are some ideas for how this could be accomplished:

• each config source could use the same config property name, and the config consumer would need to somehow specify that it wants a combined (not overwritten) value - maybe a new annotation or new API to indicate.
• each config source could use a slightly different config property name, but the config consumer could then specify wildcards (i.e. JWT could specify mp.rc.propagateHeaders.forJWT=A,B,C and Metrics could specify mp.rc.propagateHeaders.forMetrics=X,Y,Z - then MP Rest Client could inject mp.rc.propagateHeaders.*)

The latter approach is less preferred, as it could lead to config collision but it does entirely avoid the issue of overwriting vs combining.

[smillidge]

I read the referenced issue and I don't understand the use case. Are you saying that both JWT and Metrics would provide a config source for the property? Why don't you just set the property value at the umbrella spec level? In the referenced issue the set of headers required propagation could be different on different threads due to different REST clients being used.

[andymc12]

Hi @smillidge,

Are you saying that both JWT and Metrics would provide a config source for the property?

Yes, exactly.

Why don't you just set the property value at the umbrella spec level?

This is possible, but is limiting in servers that allow different MP specs to be selected. For example, a server may only support MP Rest Client and JWT out-of-the-box, but allow a user to bring their own MP OpenTracing implementation (by providing it as a library in a WAR). In this case, only the JWT headers would be specified by the umbrella-spec level (or the umbrella-spec level would specify all headers - even for MP components that are not supported).

It would also prevent third-party libraries from defining headers to be propagated. I could envision that some libraries (or user applications) might want to propagate headers that are not related to MP specs, and it would be nice if they could specify them in a config file in their library JAR or WAR, etc.

In the referenced issue the set of headers required propagation could be different on different threads due to different REST clients being used.

I don't think that is an actual requirement - I think it was implied by John's proposed solution, but my understanding is that all of the specified headers for a particular web app should be propagated on all of Rest Client instances.

Thanks for the response - please let me know if I can help further, Andy

[Emily-Jiang]

We could have a support this.

It can group all properties starting with mp.rc.propagateHeaders and append then

[struberg]

There is a bit of a low-level hack which will work portably right now. Something along like

StreamSupport.stream(config.getPropertyNames().spliterator(), false)
    .filter(n -> n.startsWith("mp.rc.propagateHeaders"))
    .flatMap(n -> Arrays.stream(config.getValue(n, String[].class)))
    .collect(Collectors.toSet());

[smillidge]

I would discourage the use of getPropertyNames() as depending on the ConfigSource this could be a very expensive operation. For example a ConfigSource backed by a distributed key value store where property names are the set of keys in the store.

Therefore I don't think wildcarding or stemming of config names is going to work as not all ConfigSources will be able to look up arbitrary key values.

[struberg]

@smillidge Yes, might be a tad bit more expensive than a simple lookup. But this will only be evaluated once at startup, right? So I don't think it will really matter.

[struberg]

Btw @andymc12 the benefit of the 2nd option in your initial post is that you could also override each value separately.

Btw, I'm not sure if I got the use case. You have a server (X). This talks to other servers (Y) using mp-rest-client, right? And it should propagate some headers it gets from a request to server X when calling Y from X? Is the straight 1:1 propagation really enough? Or might it be better to allow to even have it modified? E.g. if opentracing wants to open a new span...

Asking because that might be solved by firing a CDI event with the existing headers. Then let others modify or add something to those headers and then use this modified Map for the request to Y. Event performance should not be a big blocker. Firing 20 Mio CDI events take 1.2 seconds over here.

[Emily-Jiang]

header propagation is red haring. What @andymc12 asks is the way to get all properties with a defined prefix.

I think it can be done
String[] values= config.getValueWithPrefix("my.prefix", String[])
any property starting with my.prefix will be added to the array

or
@Inject @ConfigProperty(prefix="my.prefix") String[] myValues;

[struberg]

No @Emily-Jiang, he had a use case and we propose a solution. That might be solved with a new API. But we won't introduce an API which has ambiguous meaning and is just for a very narrow scope IF it is easily doable to resolve the use case with easy tricks.
Btw, even if we introduce that API you suggested, then how would it work internally?
I guess it would iterate over the ConfigSources and calls getPropertyNames on them, right?
This is exactly the same like my stream example above does. Only difference is that I iterate over the final set of attributes whereas you would probably filter the attributes on the single ConfigSources. But as both require to call ConfigSource#getPropertyNames() it causes the same underlying effort.

[struberg]

Btw, we could probably introduce a default method on the ConfigSource which only accesses a sub-section of all properties?

public interface ConfigSource { ...
  default Set<String> getPropertyNames(String startsWith) {
    return getPropertyNames().stream().filter(s -> s.startsWith(statsWith).collect(Collectors.toSet());
  }
}

Plus the same in Config itself (doesn't need to be a default method).

[andymc12]

@smillidge @struberg @Emily-Jiang thanks for all the feedback!

I'll give Mark's suggestion a try - I agree that it should work, and should only be executed once (at startup) so the performance expense should only be incurred once.

At this point, the folks asking for header propagation are only asking for simple propagation (not manipulation, then propagation).

Assuming Mark's suggestion works, it seems like Emily's API suggestion would basically simplify that same approach (at least from the user's perspective).

Thanks again!

[struberg]

Hi @andymc12, yes the main difference would be whether the rule is 'fixed' at bootup or whether you might even add headers dynamically based on other information separately for each request at runtime. I have no clue what your requirements are. For the first (bootup) the proposed config solution would be good enough I guess. The CDI Event solution would then allow a fully dynamic adoption of the headers during runtime.

[andymc12]

I think I would prefer to see it whenever a new Rest Client is built - so we'd probably need to use something like Emily's proposed programmatic API (String[] values= config.getValueWithPrefix("my.prefix", String[])) -- most often Rest Client instances will be built on startup, but there could be occasions where they are built on demand.

It's also probably a rare case that some dynamic component would arise and provide more config options, but not completely out of the question.

I think the net of my comment is that the API should be dynamic and look for all config values with the specified prefix on each invocation rather than caching the results at startup. I know that will have a performance impact, so the API should probably document that - we promise to use it sparingly :)

[struberg]

@andymc12 and @Emily-Jiang how would config.getValueWithPrefix("my.prefix", String[]) work internally? I mean how do you get all the values from the ConfigSources?
The only thing I could come up was again asking each ConfigSource for getPropertyNames() or getProperties(). But then you are back to the potential performance issue which some ConfigSources might have like @smillidge described.

Apart from that we should at least change the signature. It should rather return a Map<String, String> with the attribute name as key. Returning a T[] is extremely limited. And what if the type is not String? With my proposed solution it works fine and you could have one subkey return String and the other Integer. Use cases like the following would not be possible to resolve easily with the proposed API:

com.acme.servers.serverA.url=http://box1
com.acme.servers.serverA.port=9080
com.acme.servers.serverB.url=http://box2
com.acme.servers.serverB.port=9081

String[] valuesWhereIHaveNoClueWhichOnesBelongToEachotherAndType
  = config.getValueWithPrefix("com.acme.servers", String[]);

[smillidge]

I say again many potential ConfigSource implementations will not support looking up properties via a prefix as this requires key queries which are not supported on many key value stores. For example the JCache API does not have a method to get all keys simply because it is too expensive or not possible on distributed key value stores.

If you introduce the API I recommend also introducing the corresponding API into the ConfigSource so that it can optimise a query or just ignore it in the same way as some ConfigSources will have to ignore getPropertyNames() and getProperties()

[smillidge]

For example it won't be a tad more expensive than a property lookup if your ConfigSource is Redis or memcached https://redis.io/commands/keys

[struberg]

@smillidge not sure what JCache has to do with an actual ConfigSource? JCache might of course be used in a ConfigSource to access some other stuff. But JCache itself is not a ConfigSource.

If you introduce the API I recommend

Steve, can you please point me to the response where you described your solution? Could not find it. Txs!

[andymc12]

@struberg - to your point about returning a Map as opposed to a String[], I think that makes sense. Either should work for our case, but I completely agree that returning a Map of properties is generally more usable.

Thanks again!