refactor: improve telemetry api receiver listener initialization logic#2096
refactor: improve telemetry api receiver listener initialization logic#2096herin049 wants to merge 1 commit intoopen-telemetry:mainfrom
Conversation
| if err != nil { | ||
| return nil, "", err | ||
| } | ||
| addr := fmt.Sprintf("%s:%d", l.Addr().Network(), l.Addr().(*net.TCPAddr).Port) |
There was a problem hiding this comment.
Looks like l.Addr().Network() returns the network type string "tcp". So wouldn't this produce an address like tcp:53421 instead of sandbox.localdomain:53421?
That would mean the subscribe URI passed to the Telemetry API ends up as http://tcp:53421/, which Lambda wouldn't be able to resolve to deliver events to I think?
For reference, the internal listener at internal/telemetryapi/listener.go uses the listenerAddr variable directly for this and I think a lot of the logic we already have there is directly applicable here as well.
There was a problem hiding this comment.
Good catch, this must have been a typo, or I made this change very late at night. I can update this, and add a test, since I'm surprised this managed to slip through.
| func (r *telemetryAPIReceiver) Shutdown(ctx context.Context) error { | ||
| return nil | ||
| err := r.httpServer.Shutdown(ctx) | ||
| return err | ||
| } |
There was a problem hiding this comment.
Again looking at some "prior art" in internal/telemetryapi/listener.go it looks like just to be safe we might want to also nil check r.httpServer here before calling the Shutdown method on it?
2 participants
Improves the Telemetry API receiver listener initialization logic to more closely match the logic used elsewhere in the layer. Updated the default behavior to listen on
:0by default to eliminate the need of manually specifying ports and eliminate the possibility of encounteringError: address already in useerrors that are very rare and difficult to debug. The listener now handles shutdown properly by closing the HTTP listener, and several incorrect log statements have been updated with a few additional ones added.