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

Implement migration plan for selected instrumentations #2453

Open
lzchen opened this issue Apr 23, 2024 · 8 comments
Open

Implement migration plan for selected instrumentations #2453

lzchen opened this issue Apr 23, 2024 · 8 comments
Labels
enhancement New feature or request feature-request instrumentation specification Issues that are needed to comply with the specifications

Comments

@lzchen
Copy link
Contributor

lzchen commented Apr 23, 2024

A followup of #2351

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

Finished:

opentelemetry-instrumentation-requests #2002
opentelemetry-instrumentation-wsgi #2425
opentelemetry-instrumentation-flask #2454
opentelemetry-instrumentation-asgi #2610
opentelemetry-instrumentation-httpx #2631
opentelemetry-instrumentation-fastapi #2682
opentelemetry-instrumentation-aiohttp-client #2675
opentelemetry-instrumentation-django #2714
opentelemetry-instrumentation-urllib3 #2681
opentelemetry-instrumentation-urllib #2680

WIP:

opentelemetry-instrumentation-falcon #2680

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

@zhihali
Copy link
Contributor

zhihali commented Jun 22, 2024

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!

@lzchen
Copy link
Contributor Author

lzchen commented Jul 3, 2024

@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.

@evankanderson
Copy link

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.

@emdneto
Copy link
Member

emdneto commented Jul 31, 2024

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

@evankanderson
Copy link

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
Copy link

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
Copy link

evankanderson commented Aug 1, 2024

In particular, we needed open-telemetry/opentelemetry-python#3924 from 1.25.0/0.46b0.

@emdneto
Copy link
Member

emdneto commented Aug 9, 2024

Checklist for HTTP transition

  • Add semconv status as migration for urllib3 in instrumentations README
  • Populate {method} as HTTP when nonstandard method (sanitize_method)
  • 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.
  • Set schema_url based on the opt-in mode
  • Create histograms based on the opt-in mode (different metric names so no dup attributes)
  • Set metrics attributes based on the opt-in mode
  • Set span attributes based on the opt-in mode

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature-request instrumentation specification Issues that are needed to comply with the specifications
Projects
None yet
Development

No branches or pull requests

4 participants