Skip to content

Support configuration files#462

Merged
pavolloffay merged 5 commits intojaegertracing:masterfrom
pavolloffay:viper-config-files
Oct 12, 2017
Merged

Support configuration files#462
pavolloffay merged 5 commits intojaegertracing:masterfrom
pavolloffay:viper-config-files

Conversation

@pavolloffay
Copy link
Copy Markdown
Member

Support configuration files. This allows us to configure Jaeger with kubernetes config maps.

By default configuration files are disabled. Lists are currently not handled because we parse them manually. For instance cassandra.servers requires a string with comma separated values.

Follows some examples. Note that flags with dots can be written as nested objects.

{
  "query": {
    "prefix": "prefix",
    "static-files": "static"
  },
  "cassandra": {
    "keyspace": "from_file",
    "password": "pas_from_file",
    "servers": "localhost,localhost2",
    "archive": {
      "keyspace": "archive_keyspace"
    }
  },
  "es.archive.max-span-age": "75h0m0s"
}
query:
  prefix: prefix
  static-files: static
cassandra:
  keyspace: from_file
  password: pas_from_file
  servers: localhost,localhost2
  archive:
    keyspace: archive_keyspace
es.archive.max-span-age: 75h0m0s

Signed-off-by: Pavol Loffay <ploffay@redhat.com>
@ghost ghost assigned pavolloffay Oct 9, 2017
@ghost ghost added the review label Oct 9, 2017
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 100.0% when pulling 55718a7 on pavolloffay:viper-config-files into d17fad0 on jaegertracing:master.

yurishkuro
yurishkuro previously approved these changes Oct 9, 2017
Copy link
Copy Markdown
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall looks like a great feature, but it looks like it could've been done with a lot less duplication by making it the default behavior of the SharedFlags

v,
command,
flags.AddConfFileFlag,
flags.AddFlags,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason not to have config flag added as part of AddFlags? It would probably also avoid duplicating the code in L65-71 in all binaries, only do it in sFlags := new(flags.SharedFlags).InitFromViper(v)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a split for two reasons:

  1. agent does use flags.AddFlags
  2. flags.AddFlags can be specified in config file, so there would be twice sFlags := new(flags.SharedFlags).InitFromViper(v)

Copy link
Copy Markdown
Member

@yurishkuro yurishkuro Oct 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see - makes sense. In that case can we move L65-71 into that package too, so that we can have just a one liner in the mains? E.g. something like flags.TryLoadConfigFile(v, logger)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, by doing that we can make flags.ExternalConfFlags type private. I suggest renaming it to ConfigFile(Flag) ("external" is non-informative)

@yurishkuro yurishkuro dismissed their stale review October 9, 2017 18:50

deduplicate

Signed-off-by: Pavol Loffay <ploffay@redhat.com>
@coveralls
Copy link
Copy Markdown

coveralls commented Oct 11, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 912b81e on pavolloffay:viper-config-files into d17fad0 on jaegertracing:master.

v.SetConfigFile(file)
err := v.ReadInConfig()
if err != nil {
logger.Fatal(fmt.Sprintf("Fatal error config file: %s \n", file), zap.Error(err))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: don't use string formating. File name can be passed as a string field. Message could be "error loading config".

config.AddFlags(
v,
command,
flags.AddConfFileFlag,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: s/conf/config for making consistency

Signed-off-by: Pavol Loffay <ploffay@redhat.com>
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 100.0% when pulling 20697b1 on pavolloffay:viper-config-files into d17fad0 on jaegertracing:master.

Signed-off-by: Pavol Loffay <ploffay@redhat.com>
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 100.0% when pulling 5c23082 on pavolloffay:viper-config-files into d17fad0 on jaegertracing:master.

@ghost ghost assigned black-adder Oct 12, 2017
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 100.0% when pulling 5891eb2 on pavolloffay:viper-config-files into 54151f8 on jaegertracing:master.

@pavolloffay pavolloffay merged commit 0549efd into jaegertracing:master Oct 12, 2017
@ghost ghost removed the review label Oct 12, 2017
ideepika pushed a commit to ideepika/jaeger that referenced this pull request Oct 22, 2017
* Support configuration files

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

* Fix review comments

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

* rename to AddConfigFileFlag

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

* Add config file name to logs

Signed-off-by: Pavol Loffay <ploffay@redhat.com>
isaachier pushed a commit to isaachier/jaeger that referenced this pull request Nov 1, 2017
* Support configuration files

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

* Fix review comments

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

* rename to AddConfigFileFlag

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

* Add config file name to logs

Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants