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

Update trace config max defaults #268

Closed
jtescher opened this issue Oct 14, 2020 · 6 comments · Fixed by #459
Closed

Update trace config max defaults #268

jtescher opened this issue Oct 14, 2020 · 6 comments · Fixed by #459
Labels
good first issue Good for newcomers

Comments

@jtescher
Copy link
Member

See the spec change here for details open-telemetry/opentelemetry-specification#942

@jtescher jtescher added the good first issue Good for newcomers label Oct 16, 2020
@morigs
Copy link
Contributor

morigs commented Oct 21, 2020

Do I understand correctly that the task is reduced to replacing these constants with the values of environment variables (with a default value of 1000 of course)?

@jtescher
Copy link
Member Author

@morigs the conventions so far in this workspace have been to have constants (that need to be updated here) and the struct or builder to have a from_env method that would use the env vars

@morigs
Copy link
Contributor

morigs commented Oct 21, 2020

Ok. So default() should always use values of 1000.
But where from_env should be located? Is it ok to be right inside impl Config?
So usage would be Config::default().from_env()?
This will require adding .from_env () to all places where config was created, because environment variables must always be respected (imho).

@jtescher
Copy link
Member Author

@morigs I personally prefer that env vars should always be used when available as well. This project went with the explicit version both because of conventions set by the golang impl (e.g. FromEnv, WithProcessFromEnv, etc) as that is the most widely used distribution, as well as the ambiguity when using struct constructor with defaults.

E.g. Config { max_events_per_span: 100, ..Default::default() } wouldn't pick up the variables.

It does have the somewhat substantial downside that consumers of the API may be confused when they set env vars that do not have the effect they expect. I would be open to re-evaluating the decision to use explicit from env if you want to open an issue for that (e.g. we could make all fields private across the projects to force going through constructors that would pick up the environment)

@morigs
Copy link
Contributor

morigs commented Oct 21, 2020

consumers of the API may be confused when they set env vars that do not have the effect they expect

Totally agreed

if you want to open an issue for that (e.g. we could make all fields private across the projects to force going through constructors that would pick up the environment)

I believe it's better to go this way. But I'm new to the OTel ecosystem and don't know all conventions and best practices. So I rely on your decision

@frigus02
Copy link
Member

I'd be happy to move to private fields and automated environment lookup as well.

This would also enable us to add new configuration options without making that a breaking change, I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants