-
Notifications
You must be signed in to change notification settings - Fork 623
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
Remove Configuration
#1523
Remove Configuration
#1523
Conversation
5a584e8
to
8c69947
Compare
Now that we are removing the Configuration API, should we have a single place where all of the env variables are defined? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. Please update the changelog.
Hm, maybe... But how would this work? Having a file with something like this: # environment_variables.py
OTEL_PROPAGATORS = "OTEL_PROPAGATORS"
OTEL_TRACE_EXPORTER = "OTEL_TRACE_EXPORTER"
... for other Python modules to do We could do that, but is there any value in having that file? We pretty much are only defining constants that are named exactly after strings. Should we include all the environment variables defined in the spec (that would make us have to keep this hypothetical file updated with the spec)? |
910986e
to
73929d2
Compare
We should be able to generate these with that semconvgen tool eventually. The nice thing there is it updates from the spec with new/modified envvars. We can also add sphinx docstrings in the generated code to document the envvars with |
ef8974b
to
3123115
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few changes requested and some questions. Thanks for cleaning up the PR!
exporter/opentelemetry-exporter-otlp/src/opentelemetry/exporter/otlp/exporter.py
Outdated
Show resolved
Hide resolved
exporter/opentelemetry-exporter-otlp/src/opentelemetry/exporter/otlp/exporter.py
Outdated
Show resolved
Hide resolved
exporter/opentelemetry-exporter-otlp/src/opentelemetry/exporter/otlp/exporter.py
Outdated
Show resolved
Hide resolved
opentelemetry-api/src/opentelemetry/environment_variables/__init__.py
Outdated
Show resolved
Hide resolved
opentelemetry-instrumentation/src/opentelemetry/instrumentation/bootstrap.py
Show resolved
Hide resolved
I have been trying pretty much anything to fix the documentation issue. Tests pass in my laptop but fail in CI, so there will be some weird commits from my side while I try to fix this. Moving this PR to draft in the meantime, to reflect this, but feel free to leave comments if you feels like it, sorry for the inconveniences. |
Worth noting that this also fixes #1554 |
### Removed | ||
- Remove Configuration | ||
([#1523](https:/open-telemetry/opentelemetry-python/pull/1523)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably also add a changelog note about the changing variable names
### Removed | ||
- Remove Configuration | ||
([#1523](https:/open-telemetry/opentelemetry-python/pull/1523)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should also contain a note about the changing args in opentelemetry-instrument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for address my comments. I'll approve, but would like to see the updates to the changelog before merging
opentelemetry-sdk/src/opentelemetry/sdk/environment_variables/__init__.py
Outdated
Show resolved
Hide resolved
|
||
with patch("sys.argv", ["instrument", "-s", "my-service", "1", "2"]): | ||
auto_instrumentation.run() | ||
self.assertEqual(environ.get("OTEL_SERVICE_NAME"), "my-service") | ||
self.assertEqual( | ||
environ.get(OTEL_PYTHON_SERVICE_NAME), "my-service" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not in this PR scope. I believe this is used for exporter service_name
param. Shouldn't we remove this env and service_name arg and rely on Resource
attributes for service name.
OTEL_BSP_EXPORT_TIMEOUT, | ||
OTEL_BSP_MAX_EXPORT_BATCH_SIZE, | ||
OTEL_BSP_MAX_QUEUE_SIZE, | ||
OTEL_BSP_SCHEDULE_DELAY, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, didn't know you removed the _MILLIS in this PR itself. That was updated very recently in the spec. I should close #1554
This reverts commit d3fe740.
Remove
Configuration
from everywhere.Fixes #1494
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Does This PR Require a Contrib Repo Change?
Answer the following question based on these examples of changes that would require a Contrib Repo Change:
The OTel specification has changed which prompted this PR to update the method interfaces of
opentelemetry-api/
oropentelemetry-sdk/
The method interfaces of
opentelemetry-instrumentation/
have changedThe method interfaces of
test/util
have changedScripts in
scripts/
that were copied over to the Contrib repo have changedConfiguration files that were copied over to the Contrib repo have changed (when consistency between repositories is applicable) such as in
pyproject.toml
isort.cfg
.flake8
When a new
.github/CODEOWNER
is addedMajor changes to project information, such as in:
README.md
CONTRIBUTING.md
Yes. - Link to PR: Remove Configuration from instrumentations opentelemetry-python-contrib#285
No.
Checklist: