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

OpenTelemetry HTTP Attributes Breaking Changes #3892

Open
madwort opened this issue Dec 20, 2023 · 8 comments
Open

OpenTelemetry HTTP Attributes Breaking Changes #3892

madwort opened this issue Dec 20, 2023 · 8 comments

Comments

@madwort
Copy link
Contributor

madwort commented Dec 20, 2023

Email from honeycomb notifying about these changes. Not currently planned for the languages that were're currently using, but will be at some point, and I think we do use these params in some of our honeycomb boards.

e.g. http.target -> url.path AND url.query, http.status_code -> http.response.status_code, ...

see also https://opentelemetry.io/blog/2023/http-conventions-declared-stable/#migration-plan

@madwort
Copy link
Contributor Author

madwort commented Dec 20, 2023

at some point in the future, the python lib will be updated, and then the dashboards will need to be updated to reflect that

@madwort
Copy link
Contributor Author

madwort commented Dec 20, 2023

I think this is updated in v0.43b0
open-telemetry/opentelemetry-python-contrib#2002

(we are on 0.41b0 at the time of writing)

looks like if we set

os.environ["OTEL_SEMCONV_STABILITY_OPT_IN"] = "http/dup"

the code will currently emit both conventions.

@ghickman
Copy link
Contributor

ghickman commented Jan 5, 2024

just upgrade prod opentelemetry-instrumentation-django isn't picking up 0.43b0 for the django package, nor the psycopg2, or requests ones.

Running the fully qualified pip-compile command I get the same thing, no changes to requirements.prod.txt.

Dropping down to pip install --upgrade opentelemetry-instrumentation-django gives a workable error:

opentelemetry-instrumentation-django 0.41b0 requires opentelemetry-instrumentation==0.41b0, but you have opentelemetry-instrumentation 0.43b0 which is incompatible.
opentelemetry-instrumentation-dbapi 0.41b0 requires opentelemetry-instrumentation==0.41b0, but you have opentelemetry-instrumentation 0.43b0 which is incompatible.
opentelemetry-instrumentation-requests 0.41b0 requires opentelemetry-instrumentation==0.41b0, but you have opentelemetry-instrumentation 0.43b0 which is incompatible.
opentelemetry-instrumentation-psycopg2 0.41b0 requires opentelemetry-instrumentation==0.41b0, but you have opentelemetry-instrumentation 0.43b0 which is incompatible.
opentelemetry-instrumentation-wsgi 0.41b0 requires opentelemetry-instrumentation==0.41b0, but you have opentelemetry-instrumentation 0.43b0 which is incompatible.

Running this pip command with any of the opentelemetry-instrumentation-* dependencies gives a similar error, however, opentelemetry-instrumentation works. Trying to upgrade this package via pip-compile does not give any changes.

@ghickman
Copy link
Contributor

ghickman commented Jan 5, 2024

It's possible to get them all installed with pip install --upgrade opentelemetry-instrumentation opentelemetry-exporter-otlp-proto-http opentelemetry-sdk opentelemetry-instrumentation-django opentelemetry-instrumentation-psycopg2 opentelemetry-instrumentation-requests.

Copying this list over to pip-compile with pip-compile --resolver=backtracking --allow-unsafe --generate-hashes --strip-extras --output-file=requirements.prod.txt requirements.prod.in --upgrade-package opentelemetry-instrumentation --upgrade-package opentelemetry-exporter-otlp-proto-http --upgrade-package opentelemetry-sdk --upgrade-package opentelemetry-instrumentation-django --upgrade-package opentelemetry-instrumentation-psycopg2 --upgrade-package opentelemetry-instrumentation-requests still doesn't result in any changes.

@madwort
Copy link
Contributor Author

madwort commented Jan 5, 2024

Ah, so, I quickly tried to do this before I asked you & hit this problem, but assumed I was holding something wrong & you'd have a better way to do it... I just tried adding specific versions to requirements.prod.in, like this:

...
python-ulid
opentelemetry-exporter-otlp-proto-http
opentelemetry-instrumentation-django>=0.43b0
opentelemetry-instrumentation-requests>=0.43b0
opentelemetry-instrumentation-psycopg2>=0.43b0
opentelemetry-sdk
requests
...

which meant that pip-compile, instead of not upgrading them, throws a conflict instead:

% pip-compile --resolver=backtracking --allow-unsafe --generate-hashes --strip-extras --output-file=requirements.prod.txt requirements.prod.in --upgrade-package opentelemetry-instrumentation --upgrade-package opentelemetry-exporter-otlp-proto-http --upgrade-package opentelemetry-sdk --upgrade-package opentelemetry-instrumentation-django --upgrade-package opentelemetry-instrumentation-psycopg2 --upgrade-package opentelemetry-instrumentation-requests
  ERROR: Cannot install -r requirements.prod.in (line 31) because these package versions have conflicting dependencies.
Discarding opentelemetry-instrumentation-wsgi==0.41b0 (from -r requirements.prod.txt (line 677)) to proceed the resolution
  ERROR: Cannot install -r requirements.prod.in (line 31) because these package versions have conflicting dependencies.
Discarding opentelemetry-semantic-conventions==0.41b0 (from -r requirements.prod.txt (line 693)) to proceed the resolution
  ERROR: Cannot install -r requirements.prod.in (line 31) because these package versions have conflicting dependencies.
Discarding opentelemetry-util-http==0.41b0 (from -r requirements.prod.txt (line 702)) to proceed the resolution
  ERROR: Cannot install -r requirements.prod.in (line 33) because these package versions have conflicting dependencies.
Discarding opentelemetry-instrumentation-dbapi==0.41b0 (from -r requirements.prod.txt (line 661)) to proceed the resolution
  ERROR: Cannot install -r requirements.prod.in (line 31), -r requirements.prod.in (line 32), -r requirements.prod.in (line 34) and opentelemetry-instrumentation-django because these package versions have conflicting dependencies.

which we can iteratively add to pip-compile to get this:

pip-compile --resolver=backtracking --allow-unsafe --generate-hashes --strip-extras --output-file=requirements.prod.txt requirements.prod.in --upgrade-package opentelemetry-instrumentation --upgrade-package opentelemetry-exporter-otlp-proto-http --upgrade-package opentelemetry-sdk --upgrade-package opentelemetry-instrumentation-django --upgrade-package opentelemetry-instrumentation-psycopg2 --upgrade-package opentelemetry-instrumentation-requests --upgrade-package opentelemetry-instrumentation-wsgi --upgrade-package opentelemetry-semantic-conventions --upgrade-package opentelemetry-util-http --upgrade-package opentelemetry-instrumentation-dbapi --upgrade-package opentelemetry-api --upgrade-package opentelemetry-proto --upgrade-package opentelemetry-exporter-otlp-proto-common

which I think generates something workable? PR coming up

@madwort
Copy link
Contributor Author

madwort commented Jan 5, 2024

#3951

@madwort
Copy link
Contributor Author

madwort commented Jan 10, 2024

there's a question about whether to hard-code this setting or whether to add it to the dokku config - my feeling is that we won't want to change it so we should hard-code it. However, it should work when set as a dokku config, but I tried that on Friday & it didn't change the emitted telemetry. I think this is a nice-to-have, so I wouldn't be averse to parking it - leaving some notes on how to update the packages in future is a nice outcome that would help whoever may need to look at this in future.

@ghickman ghickman removed their assignment Jan 15, 2024
@madwort
Copy link
Contributor Author

madwort commented Jan 17, 2024

leaving some notes on how to update the packages in future

nb. this was done here: https://github.com/opensafely-core/job-server/pull/3951/files#diff-bd017515eb79a7fb7569b1d15e8963ea380123d4fdf779978dd4b3ab7500fd10R261-R268

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 a pull request may close this issue.

2 participants