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

HTTP semantic convention stability migration for fastapi #2682

Merged
merged 32 commits into from
Jul 12, 2024

Conversation

lzchen
Copy link
Contributor

@lzchen lzchen commented Jul 8, 2024

Part of #2453

lzchen added 21 commits April 3, 2024 15:20
@lzchen lzchen marked this pull request as ready for review July 8, 2024 23:35
@lzchen lzchen requested a review from a team July 8, 2024 23:35
@github-actions github-actions bot requested a review from ocelotl July 8, 2024 23:36
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@emdneto emdneto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reviewers: I did some runs of fastapi instrumentation using this branch and the current version available

Current version (0.46b0) - No opt-in

"attributes": {
        "http.scheme": "http",
        "http.host": "127.0.0.1:8000",
        "net.host.port": 8000,
        "http.flavor": "1.1",
        "http.target": "/foobar",
        "http.url": "http://127.0.0.1:8000/foobar",
        "http.method": "GET",
        "http.server_name": "localhost:8000",
        "http.user_agent": "curl/7.68.0",
        "net.peer.ip": "127.0.0.1",
        "net.peer.port": 40220,
        "http.route": "/foobar",
        "http.status_code": 200
    },

New version 0.47b0.dev (with the changes from this PR)

Default

"attributes": {
        "http.scheme": "http",
        "http.host": "127.0.0.1:8000",
        "net.host.port": 8000,
        "http.flavor": "1.1",
        "http.target": "/foobar",
        "http.url": "http://127.0.0.1:8000/foobar",
        "http.method": "GET",
        "http.server_name": "localhost:8000",
        "http.user_agent": "curl/7.68.0",
        "net.peer.ip": "127.0.0.1",
        "net.peer.port": 33188,
        "http.route": "/foobar",
        "http.status_code": 200
    },

New semconv

"attributes": {
        "url.scheme": "http",
        "server.address": "127.0.0.1:8000",
        "server.port": 8000,
        "network.protocol.version": "1.1",
        "url.path": "/foobar",
        "url.full": "http://127.0.0.1:8000/foobar",
        "http.request.method": "GET",
        "user_agent.original": "curl/7.68.0",
        "client.address": "127.0.0.1",
        "client.port": 45854,
        "http.route": "/foobar",
        "http.response.status_code": 200
    },

HTTP Dup

"attributes": {
        "http.scheme": "http",
        "url.scheme": "http",
        "http.host": "127.0.0.1:8000",
        "server.address": "127.0.0.1:8000",
        "net.host.port": 8000,
        "server.port": 8000,
        "http.flavor": "1.1",
        "network.protocol.version": "1.1",
        "http.target": "/foobar",
        "url.path": "/foobar",
        "http.url": "http://127.0.0.1:8000/foobar",
        "url.full": "http://127.0.0.1:8000/foobar",
        "http.method": "GET",
        "http.request.method": "GET",
        "http.server_name": "localhost:8000",
        "http.user_agent": "curl/7.68.0",
        "user_agent.original": "curl/7.68.0",
        "net.peer.ip": "127.0.0.1",
        "client.address": "127.0.0.1",
        "net.peer.port": 49964,
        "client.port": 49964,
        "http.route": "/foobar",
        "http.status_code": 200,
        "http.response.status_code": 200
    },

@emdneto
Copy link
Member

emdneto commented Jul 11, 2024

I have identified two issues that are out of the scope of this PR:

  1. We are setting url.full for server spans in opentelemetry-instrumentation-asgi. In the new semconv, this attribute isn't necessary for server spans.
  2. We are not handling exceptions in opentelemetry-instrumentation-asgi, so if exceptions occur, we won't have the error.type attribute emitted.

Will create the issues for this

@lzchen lzchen merged commit 6293d6a into open-telemetry:main Jul 12, 2024
379 checks passed
@lzchen lzchen deleted the fastpi branch July 12, 2024 18:49
lzchen added a commit that referenced this pull request Jul 25, 2024
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 this pull request may close these issues.

4 participants