Repository: open-telemetry/opamp-go
Pull request #171 (2023-06-16 21:43:02)

Replace Connection.RemoteAddr() by Connection()
I am implementing TLS in the example server and found that I need access to the underlying net.Conn. I added Connection() method to access it. Now that we have access to net.Conn, RemoteAddr() is no longer needed, it can be substituted by Connection().RemoteAddr().

Note: this is a breaking change.

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/open-telemetry/opamp-go/pull/171?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) Report
Patch coverage: **`89.47`**% and project coverage change: **`-0.77`** :warning:
> Comparison is base [(`efddaa2`)](https://app.codecov.io/gh/open-telemetry/opamp-go/commit/efddaa260895e7ebb2431deeefef3a3380d8d902?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) 76.11% compared to head [(`50dae77`)](https://app.codecov.io/gh/open-telemetry/opamp-go/pull/171?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) 75.34%.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main     #171      +/-   ##
==========================================
- Coverage   76.11%   75.34%   -0.77%     
==========================================
  Files          24       24              
  Lines        1834     1866      +32     
==========================================
+ Hits         1396     1406      +10     
- Misses        326      342      +16     
- Partials      112      118       +6     
```


| [Impacted Files](https://app.codecov.io/gh/open-telemetry/opamp-go/pull/171?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | Coverage Δ | |
|---|---|---|
| [client/types/callbacks.go](https://app.codecov.io/gh/open-telemetry/opamp-go/pull/171?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-Y2xpZW50L3R5cGVzL2NhbGxiYWNrcy5nbw==) | `66.66% <ø> (ø)` | |
| [server/httpconnection.go](https://app.codecov.io/gh/open-telemetry/opamp-go/pull/171?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-c2VydmVyL2h0dHBjb25uZWN0aW9uLmdv) | `33.33% <0.00%> (ø)` | |
| [client/internal/mockserver.go](https://app.codecov.io/gh/open-telemetry/opamp-go/pull/171?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-Y2xpZW50L2ludGVybmFsL21vY2tzZXJ2ZXIuZ28=) | `82.35% <85.71%> (+0.31%)` | :arrow_up: |
| [server/serverimpl.go](https://app.codecov.io/gh/open-telemetry/opamp-go/pull/171?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-c2VydmVyL3NlcnZlcmltcGwuZ28=) | `58.92% <94.73%> (+0.37%)` | :arrow_up: |
| [client/httpclient.go](https://app.codecov.io/gh/open-telemetry/opamp-go/pull/171?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-Y2xpZW50L2h0dHBjbGllbnQuZ28=) | `95.00% <100.00%> (+0.26%)` | :arrow_up: |
| [client/internal/httpsender.go](https://app.codecov.io/gh/open-telemetry/opamp-go/pull/171?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-Y2xpZW50L2ludGVybmFsL2h0dHBzZW5kZXIuZ28=) | `72.22% <100.00%> (+1.25%)` | :arrow_up: |
| [server/callbacks.go](https://app.codecov.io/gh/open-telemetry/opamp-go/pull/171?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-c2VydmVyL2NhbGxiYWNrcy5nbw==) | `68.18% <100.00%> (-4.55%)` | :arrow_down: |
| [server/wsconnection.go](https://app.codecov.io/gh/open-telemetry/opamp-go/pull/171?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-c2VydmVyL3dzY29ubmVjdGlvbi5nbw==) | `100.00% <100.00%> (ø)` | |

... and [2 files with indirect coverage changes](https://app.codecov.io/gh/open-telemetry/opamp-go/pull/171/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry)


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/open-telemetry/opamp-go/pull/171?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry).

tigrannajaryan: > I think the change is good. I would prefer we use the name "Connection" or "NetworkConnection" (seems redundant), rather than match the package/struct name of the connection. I will let you decide.

Just to be clear, you suggest `Connection()` as the method name instead of `NetConn()`?
andykellr: > > I think the change is good. I would prefer we use the name "Connection" or "NetworkConnection" (seems redundant), rather than match the package/struct name of the connection. I will let you decide.
> 
> 
> 
> Just to be clear, you suggest `Connection()` as the method name instead of `NetConn()`?

Yes

Patch:
diff --git a/server/httpconnection.go b/server/httpconnection.go
index e70ea82..928c62b 100644
--- a/server/httpconnection.go
+++ b/server/httpconnection.go
@@ -23,8 +23,8 @@ type httpConnection struct {
 	conn net.Conn
 }
 
-func (c httpConnection) RemoteAddr() net.Addr {
-	return c.conn.RemoteAddr()
+func (c httpConnection) Connection() net.Conn {
+	return c.conn
 }
 
 var _ types.Connection = (*httpConnection)(nil)
diff --git a/server/types/connection.go b/server/types/connection.go
index 1f8371f..2b2bd81 100644
--- a/server/types/connection.go
+++ b/server/types/connection.go
@@ -10,8 +10,8 @@ import (
 // Connection represents one OpAMP connection.
 // The implementation MUST be a comparable type so that it can be used as a map key.
 type Connection interface {
-	// RemoteAddr returns the remote network address of the connection.
-	RemoteAddr() net.Addr
+	// Connection returns the underlying net.Conn
+	Connection() net.Conn
 
 	// Send a message. Should not be called concurrently for the same Connection instance.
 	// Can be called only for WebSocket connections. Will return an error for plain HTTP
diff --git a/server/wsconnection.go b/server/wsconnection.go
index 00096c7..9ce53a4 100644
--- a/server/wsconnection.go
+++ b/server/wsconnection.go
@@ -18,8 +18,8 @@ type wsConnection struct {
 
 var _ types.Connection = (*wsConnection)(nil)
 
-func (c wsConnection) RemoteAddr() net.Addr {
-	return c.wsConn.RemoteAddr()
+func (c wsConnection) Connection() net.Conn {
+	return c.wsConn.UnderlyingConn()
 }
 
 // Message header is currently uint64 zero value.


Test patch:
diff --git a/server/serverimpl_test.go b/server/serverimpl_test.go
index fcd013b..7ebaaa3 100644
--- a/server/serverimpl_test.go
+++ b/server/serverimpl_test.go
@@ -123,7 +123,7 @@ func TestServerStartAcceptConnection(t *testing.T) {
 	assert.True(t, atomic.LoadInt32(&connectionCloseCalled) == 0)
 
 	// Verify that the RemoteAddr is correct.
-	require.Equal(t, conn.LocalAddr().String(), srvConn.RemoteAddr().String())
+	require.Equal(t, conn.LocalAddr().String(), srvConn.Connection().RemoteAddr().String())
 
 	// Close the connection from client side.
 	conn.Close()