Skip to content

Fix net peer attribute for unix socket connection#2493

Merged
lzchen merged 4 commits intoopen-telemetry:mainfrom
shadchin:fix_unix
May 30, 2024
Merged

Fix net peer attribute for unix socket connection#2493
lzchen merged 4 commits intoopen-telemetry:mainfrom
shadchin:fix_unix

Conversation

@shadchin
Copy link
Contributor

@shadchin shadchin commented May 3, 2024

Description

After switch on .get(...), KeyError exception is never raised

Type of change

Please delete options that are not relevant.

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

How Has This Been Tested?

import redis
from opentelemetry.instrumentation.redis import RedisInstrumentor

RedisInstrumentor().instrument()

client = redis.Redis(unix_socket_path="/foobar")  # or redis.Redis.from_url("unix:///foobar")
# Span attribute `net.peer.name` must be equal to `/foobar`

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

linux-foundation-easycla bot commented May 3, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Contributor

@xrmx xrmx left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, please add a test so we can avoid regressions

Copy link
Contributor

@xrmx xrmx left a comment

Choose a reason for hiding this comment

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

Thanks a lot for adding tests!

@xrmx
Copy link
Contributor

xrmx commented May 28, 2024

@shadchin care to rebase and add a changelog entry please?

@lzchen lzchen merged commit 728976f into open-telemetry:main May 30, 2024
@shadchin shadchin deleted the fix_unix branch June 1, 2024 05:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

8 participants