search

opensafely-core / job-server

A server for mediating jobs that can be run in an OpenSAFELY secure environment. q.v. job-runner
https://jobs.opensafely.org
Other
5 stars 10 forks source link

OpenTelemetry HTTP Attributes Breaking Changes #3892

Open madwort opened 9 months ago

madwort commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

I think this is updated in v0.43b0 https://github.com/open-telemetry/opentelemetry-python-contrib/pull/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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

https://github.com/opensafely-core/job-server/pull/3951

madwort commented 9 months ago

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.

madwort commented 8 months ago

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