fix(instrumentation-redis): Take host and port used for connection

[Zirak]

Which problem is this PR solving?

The Redis client allows specifying connection options in several ways, with sensible defaults. The following all translate into 127.0.0.1:6379:

createClient('redis://127.0.0.1:6379');
createClient({ host: '127.0.0.1', port: NaN });
createClient({})
createClient()

Redis somewhat normalises these separate options into its options member, and stores the properties it uses to connect to the server in connection_options. Examples of the difference between the two in the examples preceding (slightly redacted for ease of reading):

createClient('redis://127.0.0.1:6379');
// options = { port: '6379', host: '127.0.0.1' }
// connection_options = { port: 6379, host: '127.0.0.1', family: 4 }

createClient({ host: '127.0.0.1', port: NaN });
// options = { host: '127.0.0.1', port: NaN }
// connection_options = { port: 6379, host: '127.0.0.1', family: 4 }

createClient()
// options = { host: undefined }
// connection_options = { port: 6379, host: '127.0.0.1', family: 4 }

Short description of the changes

The instrumentation before this commit looks at the options property, which contains caller-supplied values before they're fully normalised and smoothed over by Redis. This means that for these weird cases, the instrumentation would populate NET_PEER_NAME and NET_PEER_PORT with erroneous values.

This commit has the instrumentation the values the Redis client uses to connect to the server, mirroring actual use.

[Zirak]

options and connection_options are both ancient - this commit in 2.5.0-1is what renamed connection_option to connection_options

Since the instrumentation targets ^2.6.0 this feels ok. If earlier is desired, we can fall back on connection_option

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.12%. Comparing base (dfb2dff) to head (002c2fb).
Report is 88 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2072      +/-   ##
==========================================
+ Coverage   90.97%   95.12%   +4.14%     
==========================================
  Files         146       10     -136     
  Lines        7492      697    -6795     
  Branches     1502      142    -1360     
==========================================
- Hits         6816      663    -6153     
+ Misses        676       34     -642     

see 137 files with indirect coverage changes

[david-luna]

@Zirak thanks for the contribution to this package

tests are failing because a formatting issue in the net.peer.port attribute. Test expects a string value instead of a number but semantic conventions says it should be a number
https://github.com/open-telemetry/opentelemetry-specification/blob/13358927bf91aa07ada15fdd2c2ab5c4c83e7628/semantic_conventions/trace/general.yaml#L42

I think the test should be changed to expect a number in net.peer.port

[Zirak]

Apologies for the delay, life tends to get in the way

@david-luna aahh I think I understand what happened; I ran the tests locally without specifying an OPENTELEMETRY_REDIS_PORT, but the CI does define it, and of course process.env is made up of strings. Fixed & tested with a cast, now it should work with both an explicit value and the default
Thanks!