opentelemetry-python-contrib icon indicating copy to clipboard operation
opentelemetry-python-contrib copied to clipboard

Implement migration plan for selected instrumentations

Open lzchen opened this issue 1 year ago • 8 comments

A followup of https://github.com/open-telemetry/opentelemetry-python-contrib/issues/2351

Tracking issue to track progress of semantic convention stability migration for the below instrumentations.

Finished:

opentelemetry-instrumentation-requests https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2002 opentelemetry-instrumentation-wsgi https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2425 opentelemetry-instrumentation-flask https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2454 opentelemetry-instrumentation-asgi https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2610 opentelemetry-instrumentation-httpx https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2631 opentelemetry-instrumentation-fastapi https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2682 opentelemetry-instrumentation-aiohttp-client https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2675 opentelemetry-instrumentation-django #2714 opentelemetry-instrumentation-urllib3 #2681 opentelemetry-instrumentation-urllib #2680 opentelemetry-instrumentation-falcon #2680

WIP:

Remaining:

opentelemetry-instrumentation-dbapi #2929 opentelemetry-instrumentation-sqlalchemy #2679 opentelemetry-instrumentation-redis #2930 opentelemetry-instrumentation-mysql #2931 opentelemetry-instrumentation-mysqlclient #2932 opentelemetry-instrumentation-psycopg #2928 opentelemetry-instrumentation-psycopg2 #2678 opentelemetry-instrumentation-pymysql #2933 opentelemetry-instrumentation-sqlite3 #2934

lzchen avatar Apr 23 '24 18:04 lzchen

Hey, @lzchen

I'm a beginner contributor, just fixed some bugs and would love to contribute more to the community. After discussing with @emdneto, we believe maybe I could try to contribute this one.

I would like to contribute to the opentelemetry-instrumentation-httplib. I read through the previous issues #2351 and #791, and read through your previous PRs: #2002, #2425, and #2454. They are really complex for me, so I think the steps to update httplib are like:

  1. __init__.py under util/opentelemetry-util-http/src/opentelemetry/util/http: Introduce Environment Variable: Add logic to check OTEL_SEMCONV_STABILITY_OPT_IN. Update Attributes: Replace old attributes with new semantic conventions. Conditional Logic: Implement conditional logic to emit old and new attributes based on the environment variable.

  2. Instrumentation File (httplib.py): Introduce Environment Variable: Similar to init.py. Update Span Attributes: Use new semantic conventions. Conditional Logic: Add logic to conditionally emit attributes.

  3. Test Files: Update Tests: Add tests to ensure both old and new conventions are correctly handled based on the environment variable.

I think my next step is to completely understand the httplib instrumentation, as I'm not really familiar with this one yet. May I ask if my steps are correct? Any other resources that could be helpful during my development?

Thank you!

zhihali avatar Jun 22 '24 22:06 zhihali

@zhihali

Thanks a lot of taking an interest in this! The steps you've outlined above sound like a great template! Another thing is to mark the package.py file to indicate the package has been migrated like here.

lzchen avatar Jul 03 '24 19:07 lzchen

An early heads-up -- we upgraded to 0.47b0, and started to encounter the following error:

ValueError: Invalid metric name: http_server_active_requests_{request}

Reading the semantic conventions, the unit for active_requests is specified as "{request}", but it's quite possible that string is interfering with a python formatting string directive. It's also possible that we've screwed up something else, but I figured I'd drop a message in here including strings like http_server_active_requests to save other people debugging time.

evankanderson avatar Jul 31 '24 19:07 evankanderson

@evankanderson, could you please raise an issue about your case with some reproducible examples? It's hard to discuss it here without details

emdneto avatar Jul 31 '24 19:07 emdneto

Yep, we're currently debugging -- we're using FastAPIInstrumentor.instrument_app with server_request_hook and excluded_urls. Rolling back to the earlier version seems to have temporarily solved the problem, but once I have a good description (probably tomorrow), I'll open a separate issue.

evankanderson avatar Jul 31 '24 19:07 evankanderson

It turns out that the issue was that we were using opentelemetry-exporter-prometheus==1.12.0rc1 with other components at 0.47b0. For some reason, dependabot did not try to upgrade that component to 1.26.0, so we were running wildly different versions of the prometheus exporter and the rest of the stack.

I'm not sure why the dependabot updates didn't work, but the issue disappeared once we upgraded the prometheus exporter.

evankanderson avatar Aug 01 '24 16:08 evankanderson

In particular, we needed https://github.com/open-telemetry/opentelemetry-python/pull/3924 from 1.25.0/0.46b0.

evankanderson avatar Aug 01 '24 16:08 evankanderson

Checklist for HTTP transition

  • [X] Add semconv status as migration for urllib3 in instrumentations README
  • [X] Populate {method} as HTTP when nonstandard method (sanitize_method)
  • [X] set request HTTP method attribute as _OTHER when nonstandard http method and for new semconv also set http.request.method_original with the original nonstandard method.
  • [X] Set schema_url based on the opt-in mode
  • [X] Create histograms based on the opt-in mode (different metric names so no dup attributes)
  • [X] Set metrics attributes based on the opt-in mode
  • [X] Set span attributes based on the opt-in mode

emdneto avatar Aug 09 '24 19:08 emdneto