Fix client address is set to server address in new semconv by y-young · Pull Request #3354 · open-telemetry/opentelemetry-python-contrib

y-young · 2025-03-11T14:53:22Z

Description

With OTEL_SEMCONV_STABILITY_OPT_IN=http, the client.address in server span is wrongly set to server address.

It it because the middleware calls _set_http_host_server and the client.address is set for the first time to server address here:

opentelemetry-python-contrib/opentelemetry-instrumentation/src/opentelemetry/instrumentation/_semconv.py

Line 357 in 3708604

set_string_attribute(result, CLIENT_ADDRESS, host)

When it calls _set_http_peer_ip_server later on, since the client.address is already set, this has no effect:

opentelemetry-python-contrib/opentelemetry-instrumentation/src/opentelemetry/instrumentation/_semconv.py

Lines 368 to 371 in 3708604

    
           if _report_new(sem_conv_opt_in_mode): 
        
               # Only populate if not already populated 
        
               if not result.get(CLIENT_ADDRESS): 
        
                   set_string_attribute(result, CLIENT_ADDRESS, ip)

This was changed in #2814.

Fixes #3356.

Type of change

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Tested with a minimal FastAPI application: https://github.com/y-young/opentelemetry-fastapi-test

Without patch

Without `OTEL_SEMCONV_STABILITY_OPT_IN`

Scope: {'type': 'http', 'asgi': {'version': '3.0', 'spec_version': '2.3'}, 'http_version': '1.1', 'server': ('127.0.0.1', 8000), 'client': ('127.0.0.1', 54737), 'scheme': 'http', 'root_path': ''}

Span:

{
    "name": "GET /",
    "context": {
        "trace_id": "0x3d94c2edf94a8c68e66a261883939a70",
        "span_id": "0x26a8e38e79afe0ae",
        "trace_state": "[]"
    },
    "kind": "SpanKind.SERVER",
    "parent_id": null,
    "start_time": "2025-03-11T14:42:33.575716Z",
    "end_time": "2025-03-11T14:42:33.582507Z",
    "status": {
        "status_code": "UNSET"
    },
    "attributes": {
        "http.scheme": "http",
        "http.host": "127.0.0.1:8000",
        "net.host.port": 8000,
        "http.flavor": "1.1",
        "http.target": "/",
        "http.url": "http://127.0.0.1:8000/",
        "http.method": "GET",
        "http.server_name": "127.0.0.1:8000",
        "http.user_agent": "",
        "net.peer.ip": "127.0.0.1",
        "net.peer.port": 54737,
        "http.route": "/",
        "http.status_code": 200
    },
    "events": [],
    "links": [],
    "resource": {
        "attributes": {
            "service.name": "test-app"
        },
        "schema_url": ""
    }
}

The net.peer.ip and net.peer.port here is correct.

With `OTEL_SEMCONV_STABILITY_OPT_IN`

Set os.environ["OTEL_SEMCONV_STABILITY_OPT_IN"] = "http" and restart the app:

Scope: {'type': 'http', 'asgi': {'version': '3.0', 'spec_version': '2.3'}, 'http_version': '1.1', 'server': ('127.0.0.1', 8000), 'client': ('127.0.0.1', 50644), 'scheme': 'http', 'root_path': ''}

Span:

{
    "name": "GET /",
    "context": {
        "trace_id": "0xc37fa0f9c920df31a3f6966771f70ce4",
        "span_id": "0x8799fdefa09e60b1",
        "trace_state": "[]"
    },
    "kind": "SpanKind.SERVER",
    "parent_id": null,
    "start_time": "2025-03-11T14:39:09.180212Z",
    "end_time": "2025-03-11T14:39:09.185639Z",
    "status": {
        "status_code": "UNSET"
    },
    "attributes": {
        "url.scheme": "http",
        "client.address": "127.0.0.1:8000",
        "server.port": 8000,
        "network.protocol.version": "1.1",
        "url.path": "/",
        "http.request.method": "GET",
        "user_agent.original": "",
        "client.port": 50644,
        "http.route": "/",
        "http.response.status_code": 200
    },
    "events": [],
    "links": [],
    "resource": {
        "attributes": {
            "service.name": "test-app"
        },
        "schema_url": ""
    }
}

The client.address is set to 127.0.0.1:8000, which is the server address.

Install patched local version

uv add ../opentelemetry-python-contrib

Scope: {'type': 'http', 'asgi': {'version': '3.0', 'spec_version': '2.3'}, 'http_version': '1.1', 'server': ('127.0.0.1', 8000), 'client': ('127.0.0.1', 62487), 'scheme': 'http', 'root_path': ''}

Span:

{
    "name": "GET /",
    "context": {
        "trace_id": "0xc669651b2543f96d4abc362360ff76da",
        "span_id": "0x79ed73755c5bee36",
        "trace_state": "[]"
    },
    "kind": "SpanKind.SERVER",
    "parent_id": null,
    "start_time": "2025-03-11T14:33:58.230421Z",
    "end_time": "2025-03-11T14:33:58.236676Z",
    "status": {
        "status_code": "UNSET"
    },
    "attributes": {
        "url.scheme": "http",
        "server.address": "127.0.0.1:8000",
        "server.port": 8000,
        "network.protocol.version": "1.1",
        "url.path": "/",
        "http.request.method": "GET",
        "user_agent.original": "",
        "client.address": "127.0.0.1",
        "client.port": 62487,
        "http.route": "/",
        "http.response.status_code": 200
    },
    "events": [],
    "links": [],
    "resource": {
        "attributes": {
            "service.name": "test-app"
        },
        "schema_url": ""
    }
}

The client.address is correct now.

Does This PR Require a Core Repo Change?

Yes. - Link to PR:
No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

Followed the style guidelines of this project
Changelogs have been updated
Unit tests have been added
Documentation has been updated

linux-foundation-easycla · 2025-03-11T14:53:28Z

The committers listed above are authorized under a signed CLA.

✅ login: y-young / name: Googleplex (79bd441, de843c6, 4462f45, 61ada6c, 96c74c3, dc434f7)
✅ login: emdneto / name: Emídio Neto (d1a6c52)
✅ login: lzchen / name: Leighton Chen (3559c67)

tammy-baylis-swi · 2025-03-11T15:57:40Z

Thanks @y-young for the description and sample app.

I think the change makes sense based on semconv for CLIENT_ADDRESS and SERVER_ADDRESS. The detail addressed by the change is just not covered in the migration guide.

The ASGI unit tests are failing as the expected are also missing server.address. Please could these be updated? As well as the Changelog.

tammy-baylis-swi · 2025-03-11T17:23:55Z

Also, for paper trail please could you create a new bugfix Issue and link to this PR?

emdneto

I remember discussing #2814 with @lzchen and there was a good reason for that. Will try to remember the details

lzchen · 2025-03-17T13:25:16Z

@emdneto

I remember discussing #2814 with @lzchen and there was a good reason for that. Will try to remember the details

I can't remember the reasoning for having http.host map to client.address, this was not properly defined in the migration guide. The deprecation notice says: Deprecated, use one of server.address, client.addressorhttp.request.header.host instead, depending on the usage.

emdneto · 2025-03-17T13:40:20Z

@emdneto

I remember discussing #2814 with @lzchen and there was a good reason for that. Will try to remember the details

I can't remember the reasoning for having http.host map to client.address, this was not properly defined in the migration guide. The deprecation notice says: Deprecated, use one of server.address, client.addressorhttp.request.header.host instead, depending on the usage.

I remember the deprecation notice; that's why I thought it made sense. Thanks @lzchen for the double-check

Fix client address is set to server address

de843c6

y-young requested a review from a team as a code owner March 11, 2025 14:53

y-young added 2 commits March 12, 2025 00:58

fix: asgi tests

79bd441

docs: update CHANGELOG

96c74c3

y-young changed the title ~~Fix client address is set to server address when semconv stability is opted in~~ Fix client address is set to server address in new semconv Mar 11, 2025

emdneto reviewed Mar 11, 2025

View reviewed changes

lzchen reviewed Mar 17, 2025

View reviewed changes

Comment thread opentelemetry-instrumentation/src/opentelemetry/instrumentation/_semconv.py Outdated

lzchen reviewed Mar 17, 2025

View reviewed changes

Comment thread CHANGELOG.md

y-young added 3 commits March 18, 2025 22:05

Merge branch 'main' into fix/client-address

4462f45

docs: update CHANGELOG

61ada6c

refactor: only populate server address if missing

dc434f7

y-young requested a review from lzchen March 19, 2025 15:16

Merge branch 'main' into fix/client-address

d1a6c52

emdneto approved these changes Mar 21, 2025

View reviewed changes

lzchen approved these changes Mar 21, 2025

View reviewed changes

Merge branch 'main' into fix/client-address

3559c67

xrmx enabled auto-merge (squash) March 24, 2025 14:17

xrmx merged commit af17965 into open-telemetry:main Mar 24, 2025

emdneto mentioned this pull request Apr 24, 2025

ASGI instrumentation metrics don't align with the semantic conventions #3310

Closed

pmcollins pushed a commit to pmcollins/opentelemetry-python-contrib that referenced this pull request Sep 9, 2025

record_exception: Accept BaseException (open-telemetry#3354)

c2fc945

Navigation Menu

Repository navigation

Fix client address is set to server address in new semconv - #3354

Fix client address is set to server address in new semconv#3354
xrmx merged 8 commits into
open-telemetry:mainfrom
y-young:fix/client-address

y-young commented Mar 11, 2025 •

edited

Loading

Uh oh!

linux-foundation-easycla Bot commented Mar 11, 2025 •

edited

Loading

Uh oh!

tammy-baylis-swi commented Mar 11, 2025

Uh oh!

tammy-baylis-swi commented Mar 11, 2025

Uh oh!

emdneto left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

lzchen commented Mar 17, 2025

Uh oh!

emdneto commented Mar 17, 2025

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	if _report_new(sem_conv_opt_in_mode):
	# Only populate if not already populated
	if not result.get(CLIENT_ADDRESS):
	set_string_attribute(result, CLIENT_ADDRESS, ip)

Conversation

y-young commented Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Without patch

Without OTEL_SEMCONV_STABILITY_OPT_IN

With OTEL_SEMCONV_STABILITY_OPT_IN

Install patched local version

Does This PR Require a Core Repo Change?

Checklist:

Uh oh!

linux-foundation-easycla Bot commented Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tammy-baylis-swi commented Mar 11, 2025

Uh oh!

tammy-baylis-swi commented Mar 11, 2025

Uh oh!

emdneto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lzchen commented Mar 17, 2025

Uh oh!

emdneto commented Mar 17, 2025

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

y-young commented Mar 11, 2025 •

edited

Loading

Without `OTEL_SEMCONV_STABILITY_OPT_IN`

With `OTEL_SEMCONV_STABILITY_OPT_IN`

linux-foundation-easycla Bot commented Mar 11, 2025 •

edited

Loading

emdneto left a comment •

edited

Loading