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

Define OTEL_CONFIG_FILE with environment variable merge semantics #3840

Conversation

jack-berg
Copy link
Member

An alternative to #3805 which merges the environment variable configuration scheme instead of ignoring it.

The merge semantics are messy. As discussed in #3805, there are no merge rules that will satisfy everyone in all use cases. The rules in this PR reflect @trask's proposal, which aims to be the least surprising.

Resolves #3752.

|----------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| OTEL_SDK_DISABLED | `.disabled` | Replaces model. |
| OTEL_RESOURCE_ATTRIBUTES | `.resource.attributes` | Merges with model, taking higher priority. |
| OTEL_SERVICE_NAME | `.resource.attributes."service.name"` | Replaces model. Takes priority over `OTEL_RESOURCE_ATTRIBUTES`. |
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems very arbitrary and hard to maintain and keep up to date. There is an alternative that can save us from having to define these rules.

Copy link
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

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

I agree with the statement in the descripiton of this PR: The merge semantics are messy. As discussed in #3805, there are no merge rules that will satisfy everyone in all use cases

Because of that, I think we should reconsider trying to define these rules at all, I left a link for an alternative solution in a comment.

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

All,

Let's compare the two proposed solutions,
and see how it would work.

PR #3805 uses only the configuration file.
Environment variables are not used, beside OTEL_CONFIG_FILE.
This proposal is referenced as (F).

PR #3840 uses the configuration file,
with additional merges from environment variables.
This proposal is referenced as (M)

As a user deploying otel, I have a working file configuration for my application.

Now, I deploy the config on two different machines.

With (F), it is guaranteed to work the same way.

With (M), it may work on one machine and fail on another,
because some environment variables are set or unset or set differently.

As a user deploying otel, I have a working file configuration for my
application A, written in say java.

I want to deploy another application B, written in say C++.

With (F), it is guaranteed to work the same for both applications.

With (M), it will break, because the list of environment variables
supported by SDK in various languages is not the same.
Java might honor a variable that C++ does not support.

As a user writing the configuration file,
I expect clarity from the configuration file, with no surprises.

The configuration file supports environment variables substitution already.

For example, given the following line:

  schedule_delay: ${OTEL_BSP_SCHEDULE_DELAY}

With (F),
If I see this line in the config file, I expect the environment variable to
be used.

If I do not see this line, I expect the environment variable to be not used.

With (M), if the line is not present in the config file,
the environment variable can still be used by the SDK implementation.

Variable names should not be magic.

Given this line:

  schedule_delay: ${MY_SCHEDULE_DELAY}

(F) allows to use any variable name, or falls to the schema default when the
line is missing.

With (M), when the line is missing,
the environment variable used is ${OTEL_BSP_SCHEDULE_DELAY},
which imply to research this further in the doc,
and imply differences between SIG (for example, C++ does not support this one).

The configuration models, between the file configuration in yaml,
and the environment variable configuration, with a flat list of name/values,
are different.

With a file configuration, a trace exporter can have several processors,
which imply several exporters, etc.

With a environment variable configuration, there is only one exporter
supported: variables are named OTEL_EXPORTER_OTLP_TRACES_ENDPOINT, not
OTEL_EXPORTER_OTLP_TRACES_ENDPOINT_1, OTEL_EXPORTER_OTLP_TRACES_ENDPOINT_2, etc.

(F) allows to use the full expressiveness of the model (a tree)

(M) restricts to a less expressive model (a list), and can not represent
everything (F) can.

With (F), all environment variables are supported at once,
because the file parser knows how to implement environment variables
substitution. No code change is needed to have one more variable in the
configuration file, used as a substitution for a node.

With (M), each environment variable has to be explicitly supported by code
in the SDK, and coverage by various SIG can vary.


For all the reasons exposed above, overall the proposal (M) does not look like
an appealing solution.

Please abandon this PR (M), and merge PR #3805 (F) instead.

@jack-berg
Copy link
Member Author

Closing this PR, as I now believe that #3850 is the best way to go for the reasons discussed here.

@marcalff thanks for those very useful examples. I think the introduction a new environment variable scheme provides a clean slate that prevents a lot of the issues you discuss above.

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.

Should file configuration merge environment variable configuration?
3 participants