Open oxzi opened 11 months ago
Why do you favor using RuntimeConfig
over adding a new type just for that purpose? Is it more than it's already there and has all the members?
So far the purpose of RuntimeConfig
is to be a copy of the config stored in the database and handle updates of that. For this, it embeds ConfigSet
from which it gains a few members that aren't directly obvious from the struct definition and also has methods like for example RLock()
. With the proposed change, I think it would be less obvious what belongs together, so I think I'd favor keeping a separate type for the in-memory representation of the config from the database.
In a nutshell, all state variables were purged from the struct (might still be passed to other, non updated types).
Would you keep them that way, i.e. it's passed as a function parameter for that specific function, keep it as a parameter? Or would you want to eliminate all passing around of these values in function parameters?
Why do you favor using
RuntimeConfig
over adding a new type just for that purpose? Is it more than it's already there and has all the members?
Mostly because it is already there and the name is somewhat fitting.
So far the purpose of
RuntimeConfig
is to be a copy of the config stored in the database and handle updates of that. For this, it embedsConfigSet
from which it gains a few members that aren't directly obvious from the struct definition and also has methods like for exampleRLock()
. With the proposed change, I think it would be less obvious what belongs together, so I think I'd favor keeping a separate type for the in-memory representation of the config from the database.
I would agree with that. Otherwise, two different things would coincide here, which actually have nothing to do with each other. Pardon.
In a nutshell, all state variables were purged from the struct (might still be passed to other, non updated types).
Would you keep them that way, i.e. it's passed as a function parameter for that specific function, keep it as a parameter? Or would you want to eliminate all passing around of these values in function parameters?
No, I would strictly enforce this change in the whole code to always signal when the shared state is being used.
In this concrete case, I only did a partial implementation just for the listener.Listener
- mostly by replacing the occurrences of the removed state variables with the singleton access method.
This side note was meant to say that in a final implementation the individual state variables are not passed through at redundant places.
- obj, err := object.FromEvent(ctx, l.db, &ev)
+ obj, err := object.FromEvent(ctx, &ev)
For example, this line would more look like this, as object.FromEvent
itself would get the database reference in its method scope.
I just remembered that the object and incident caches are already realized as global variables at the moment. https://github.com/Icinga/icinga-notifications/blob/4044b188a9f9423be532d11fc0c70b313fc0ca39/internal/object/object.go#L21-L24 https://github.com/Icinga/icinga-notifications/blob/4044b188a9f9423be532d11fc0c70b313fc0ca39/internal/incident/incidents.go#L16-L19
Those should probably also be taken into account as well. So if we realize the suggested approach, this would probably mean moving them to a type with the functions that use those variables and then adding pointers to those instances as well. And my feeling is that by having a central place for all those variables that are used pretty much everywhere, at some point, we'll run into cyclic imports.
So I wouldn't rule out using individual variables near where the actual types are defined (maybe similar to for example DefaultServeMux
in net/http
).
Given that you already show some diffs, I presume you're trying some things out locally. Have you tried other approaches so far were you maybe ran into problems?
I just remembered that the object and incident caches are already realized as global variables at the moment.
Those should probably also be taken into account as well. So if we realize the suggested approach, this would probably mean moving them to a type with the functions that use those variables and then adding pointers to those instances as well. And my feeling is that by having a central place for all those variables that are used pretty much everywhere, at some point, we'll run into cyclic imports.
So I wouldn't rule out using individual variables near where the actual types are defined (maybe similar to for example
DefaultServeMux
innet/http
).
I would agree as those global variables are bound to their package's scope and are only being used in one resp. two functions.
They are not used from the outside and are more like C static
variables and are not relevant for external callers.
Thus, and also considering cyclic imports, I would only introduce the new proposed behavior for those global or state variables which are being referenced from different packages and are (at least somewhat) derived from the configuration file.
Thanks for bringing up those global variables; I have just checked all other global variables, but found none which I would also extract for the reasons stated above.
Given that you already show some diffs, I presume you're trying some things out locally. Have you tried other approaches so far were you maybe ran into problems?
For one, I played with normal global variables, but didn't find the access signaled clearly enough there. Imho, a getter method - although technically pointless - shows more clearly that it should also be accessed this way elsewhere.
Likewise, I toyed with a common struct, similar to the one proposed here, but which was passed through.
This makes the data binding more explicit, but the actual problem of the hundred references remains.
I tried to extend this idea by passing a context.Context
with explicit state values, but this just felt unnecessary complicated.
Besides that, I looked at how other frameworks do it, e.g. viper. There you have getter methods on the loaded configuration based on the value's key, which however ends up being similar, albeit more complex, to the suggestion made here.
Looking for other inspiration online, I often found a great focus on "properly" creating the state from one or many configs, but in the end you should draw the end of the owl yourself. Especially large applications often outsourced the problem in such a way that State is external, e.g. in an etcd, and elsewhere a mixture of passing references and the singleton pattern proposal made here.
In this issue I would like to point out the potential problems of the implicit state variables, which are created in the main function and propagated from there throughout the program, and propose a change by using a single state.
State Survey
Brief listing which variables are being generated on startup and being propagated to other components.
There is currently a logical coupling between the identified variables, all holding a central state. However, there is no central union type and, furthermore, the variables (reference) are just being passed, even multiple times.
cmd/icinga-notifications-daemon/main.go
Those four variables are generated in the main function and are being passed to the listed functions and thereby throughout the whole codebase.
*config.ConfigFile
db := conf.Database.Open
listener.NewListener
*logging.Logging
db := conf.Database.Open
runtimeConfig := config.NewRuntimeConfig
listener.NewListener
*icingadb.DB
runtimeConfig := config.NewRuntimeConfig
listener.NewListener
*config.RuntimeConfig
listener.NewListener
config.RuntimeConfig
There is exactly one
RuntimeConfig
instance, generated byconfig.NewRuntimeConfig
in themain.go
file as listed above.The
RuntimeConfig
already holds references to the other three state variables.listener.Listener
With referencing all four state variables, the
Listener
keeps unnecessary duplicate references. If theRuntimeConfig
fields would be public, theListener
could already dropdb
andlogs
.From there on, the variables will be passed on as the following:
conf
variable will be used to eitherConfigFile.Listen
and.DebugPassword
field or*incident.Incident
by passing all state arguments further.logs
is only in use for the sameIncident
line.db
will be used to*object.Object
from an*event.Event
- which itself also keeps a reference -,Event
andIncident
as listed for theconf
above.runtimeConfig
is in useInicident
as all other state variables andConfigSet
struct.object.Object
Within an
object.Object
, thedb
reference is used to create statements.incident.Incident
An
*incident.Incident
will be created resp. fetched through theGetCurrent
function referred multiple times in thelistener.Listener
section above. This method stores multipleIncident
s based on their*object.Object
, each holding all state variables exceptlogs
.While the
logs
variable is being passed into theincidents.GetCurrent
function, it is used there to generate anlogging.Logger
to be stored for eachIncident
.conf
variable is read for its.ChannelPluginDir
and.Icingaweb2URL
values, being passed to a*channel.Channel
extracted from theruntimeConfig
.db
is also used for statement generation andIncidientRow
synchronization.runtimeConfig
is used to read and alter its stored values.However, it does not seem like the state variables will be propagated further from this point on.
Suggested Change
First, minimize the amount of state variables to one single variable, holding all information. As mentioned above, the
config.RuntimeConfig
can be altered accordingly.With this small change to the
RuntimeConfig
, it can be used as the single state variable in other places.Next, the hundreds of references to the very same instance should be purged. As the other packages are already referring to the
config
package, a dependency does already exist. Thus, a unique state can be placed there, e.g., by following the singleton pattern.Both generation and access to the
RuntimeConfig
might look like the following:This pattern just adds a bit of sugar around a simple global variable. For testing, however, mocking the
RuntimeConfig
is still easy as it can simply be altered.The effects might look, when only changing the
listener.Listener
for this example's sake, like this:In a nutshell, all state variables were purged from the struct (might still be passed to other, non updated types). The
config.GetRuntimeConfig()
call makes it, imho, easier to signal resp. identify a state change from this package's or type's internal data to some common state.Implications
While less references to the very same data flies around, still the same data is being modified. Furthermore, it might be easier to spot the difference between "local" and "shared" data.