Should caching parameters go to CLI parameters or Configuration parameters?

[garypen]

Is your feature request related to a problem? Please describe.

(Preserving the discussion about caching parameters here for further discussion...)

This should go in Configuration

I'm not convinced this is the right name for this field...

This variable in particular concerns the cache size of the query planning. There will probably be more caches. For example the queries will be parsed to Query objects Soon ™️ and it's also kind of a query cache.

I think we need to be more specific. For example we could create a struct Cache (in module configuration) that will have a field for each different cache. What do you think?

Member
Author
@garypen garypen 1 hour ago
I know what you mean about the name and the fact that it should be in configuration (which didn't exist before my merge).
Name suggestion: plan_cache_limit (to identify that it is only for plans). I could make that change in this PR.

The bigger suggestion of changing to Configuration and the Cache struct should be in a separate Issue/PR I think, since it is probably expanding the scope of this one too much.

Member
@cecton cecton 20 minutes ago
Configuration exists for a very long time now 🤔 even before I arrived.

Name suggestion: plan_cache_limit (to identify that it is only for plans).

Sounds good to me

The bigger suggestion of changing to Configuration and the Cache struct should be in a separate Issue/PR I think, since it is probably expanding the scope of this one too much.

I don't think we are in a hurry and since you are introducing those I would prefer that it is included in this PR. Things that are out-of-scope imo should be things that exist already in the code and should be improved in some way. But the parameter plan_cache_limit is something introduced in this PR so it makes sense to me that this PR locates it as best it could. I'm pretty sure this will also simplify your code.

It is possible there is a reason I'm not seeing here that makes this change complicated somehow so feel free to merge if that's the case.

@garypen
Sorry, that was my mis-apprehension. I was trying to refer to the changes you had made to internal parameters yesterday.

I could add it as a configuration, but I slightly prefer a command line and I think it could be resolved in a later issue/PR.

I think there's a good discussion here which will keep to a later issue/PR, so I will merge this and preserve the discussion in issue

Describe the solution you'd like

A discussion about how to proceed.

Describe alternatives you've considered

Configuration vs Options

Additional context
N/A

[cecton]

I'm pretty sure @BrynCooke will want these parameters to go into Configuration because it's easier to maintain. As a general rule of thumb, I use CLI parameters only for things that are interesting to override. For example the log level (env_filter parameter). But there is no strict rule about this at the moment and I think we (the team) are still figuring out what to put where.

Now you said:

I slightly prefer a command line

If you don't mind sharing your thoughts on why you would want those parameters to be CLI parameters instead of Configuration file parameters?

In my opinion the caching parameters are part of the configuration of the router itself. I might want to share this configuration file between multiple machines so it behaves the same. Using CLI parameters is less scalable as you need to change the command invocation.

I imagine 2 possible scenarios:

1. If router is running inside a Docker image, having to change the command line arguments is annoying for the user. They would rather use the configuration file instead. It even more relevant for k8s with the configmaps. (This is an argument for the configuration file)
2. If router is running as a service of systemd, having them in CLI arguments will allow to configure the service per machine without having to change the configuration file. (This is an argument for the CLI arguments)

Now I do have a proposition:

The struct Configuration still needs to be the only source of truth but its values can be overridden by CLI arguments. This will require a bit of rework around the structs ConfigurationKind and ConfigurationStream but I think this is valuable. This should be done in a separate PR as you said.

[garypen]

My original comment wasn't clear. I should have noted that in this particular case, I don't want the cache to be re-sizeable at run-time, so making it a startup option rather than configuration (which is hot loadable here) seems desirable.

There are rust libraries that support override configuration and I've had a good experience with this approach in the past, so it's certainly an option.

If we can avoid configuration/options completely, I think that's preferable. In this case, I feel like we might be able to auto-tune cache sizes at some point based on eviction rates, but having an option that we later deprecate seems like a good step for now.

[Geal]

IIRC the cache is recreated on configuration change, so changing it in the configuration file that is hot reloadable is fine here. And that definitely looks like an option that would be interesting to tweak without having to restart the process (because then you need a failover server, time for graceful shutdown, etc)

[BrynCooke]

I am a -1 on having any configuration or command line parameter for this at this moment.
We should be able to come up with a sensible default, and if we see a need to add config for this later we can.

For us to add configuration options the things we should ask ourselves are:

1. Will the user understand this option without looking at the code?
2. Is the benefit of allowing configuration of this going to be more than the cost of increased complexity?
3. What are the steps the user need to take to set this configuration correctly? Are there specific metrics that the user can look at? If not then how can the user meaningfully decide on a good value.
4. Is a good default going to be suitable for 99% of users?

In this case I would begin with a sensible default and re-evaluate if the face of an actual need to configure this.

If we do need to configure would suggest that configuration.yaml would be the single place to have this for now. Kubernetes is our primary target platform, and the market share of non-Kubernetes deployments is likely to decrease over time rather than grow.

[garypen]

There is a default, but I don't "know" that it's sensible. I tried to find some prior art, but failed and I don't have any intuition for a sensible default. So, the default is 100 and it's overridable via command line option. That seems like a conservative approach for now, since we can later deprecate and perform auto tuning based on eviction rates.

[BrynCooke]

Seems reasonable, but in this case perhaps we should prefix this parameter with experimental-? Users will know that it may not stay.

[garypen]

I think the industry standard approach is to not document such options, but I suppose that doesn't work too well with open source! :)

I only saw this after I merged my changes, so that's a bad thing. I suppose that once we finish this discussion, we'll need to do some retroactive fixing and this will be part of it.

[Geal]

yep there could be a "here be dragons" part of the configuration file for which we don't commit on support, but still can tweak for now

[Geal]

2 things on finding a good default value:

• apollo studio has that information for our customers. It has a list of common queries or common requested fields, and we can see the difference per customer
• we should add a metric for this, that we can collect and analyze (so we might need to collect metrics independently from the tracing)

[Geal]

as follow up on #118 (comment) how about environment variables for overridable things. Note that there are already environment variables for different things, like internal parameters for OTLP ( #50

router/crates/apollo-router/src/configuration/otlp/mod.rs

Line 53 in 030ce44

match std::env::var("ROUTER_TRACING").as_deref() {

) or jaeger credentials (

router/crates/apollo-router/src/configuration/mod.rs

Lines 226 to 232 in 030ce44

	fn default_jaeger_username() -> Option<String> {
	std::env::var("JAEGER_USERNAME").ok()
	}
	fn default_jaeger_password() -> Option<String> {
	std::env::var("JAEGER_PASSWORD").ok()
	}

).

I'm not suggesting we should have all 3 methods though, I'd just like us to be consistent and have like one main way and one override way

[cecton]

I personally use env variable for secret during development. This is a common practice, example of PG_USER and PG_PASSWORD for postgres.

Having the secrets in the config file doesn't seem like a good idea for production but I guess some will prefer that.

Maybe it's best to be flexible and allow all 3 options.

[cecton]

Wait this is the wrong discussion for secrets omg.

[cecton]

@BrynCooke here you go #197

[cecton]

I think it will be nice to have env variable to override some things (like log level). But it's hard to do much with them.

[BrynCooke]

OK, I'm going to copy paste my reply above

[cecton]

I agree with @Geal that env variables would be much better for now.

I suggest we delete the CLI parameters related to caching and use (build?) environment variable instead.

Vote for me 🇧🇪

[Geal]

runtime env variables are fine. Once people start testing, we'll be able to tell them "could you try with VAR_THAT_FIXES_EVERYTHING" and they won't have to rebuild

[cecton]

fair