Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document environment variable names #11

Open
pavolloffay opened this issue Nov 4, 2020 · 5 comments
Open

Document environment variable names #11

pavolloffay opened this issue Nov 4, 2020 · 5 comments

Comments

@pavolloffay
Copy link
Member

The properties in the config should be overridable by the environment variables (and system properties e.g. in Java). The names of the variables should be documented and consistent across clients.

@pavolloffay
Copy link
Member Author

Go SDK https://github.com/hypertrace/goagent/tree/main/config#agent-config-go already supports some env vars. It can be a good starting point.

@pavolloffay pavolloffay changed the title Document environment variables Document environment variable names Nov 4, 2020
@jcchavezs
Copy link
Contributor

The precende (at least in goagent) is

  • in-code values
  • env vars
  • config file values
  • defaults

So env vars override config file values and defaults. The naming we took in go was to flatten the keys in the config structure. For example: config.dataCapture.httpHeaders.request = true can be loaded through HT_DATA_CAPTURE_HTTP_HEADERS_REQUEST=true. We can add a tool for generating this list of env values in DEFAULTS.md and check they are updated on every PR.

@pavolloffay
Copy link
Member Author

The precedence order looks fine.

Another topics:

  • Shall we fail at startup of config file cannot be load? - I would say yes
  • Shall we set a default service name if not specified? - I would also say yes - "default_service_name"?

@pavolloffay
Copy link
Member Author

For example: config.dataCapture.httpHeaders.request = true can be loaded through HT_DATA_CAPTURE_HTTP_HEADERS_REQUEST=true. We can add a tool for generating this list of env values in DEFAULTS.md and check they are updated on every PR.

We should consider changing this to HT_DATACAPTURE.HTTPHEADERS_REQUEST it directly maps to the config object and the transformation is easier:

  1. make it all uppercase
  2. change dots to _.

@jcchavezs
Copy link
Contributor

Shall we fail at startup of config file cannot be load? - I would say yes

I'd prefer to not to fail when it comes to instrumentation. That would cause the so called observed effect. Instead I would try to run normally and drop a log line with the error.

Shall we set a default service name if not specified? - I would also say yes - "default_service_name"?

I think unknown is probably a better default service as it represents the actual problem.

We should consider changing this to HT_DATACAPTURE.HTTPHEADERS_REQUEST it directly maps to the config object and the transformation is easier:

  1. make it all uppercase
  2. change dots to _.

I think not making camelcase elements like dataCapture or httpHeaders into snake case makes it less readable and I would not sacrifice that for the convenience of parsing. Also I am not sure if you intented to add that dot in the middle of DATACAPTURE.HTTPHEADERS, probably not?

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

No branches or pull requests

2 participants