-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[exporter/syslog]: fix setting network connection #31202
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested with both UDP and TCP, works like a charm 👌 Thank you!
5de5123
to
9d54ac1
Compare
@@ -78,8 +78,8 @@ func (s *sender) dial() error { | |||
s.conn = nil | |||
} | |||
var err error | |||
if s.tlsConfig != nil { | |||
s.conn, err = tls.Dial("tcp", s.addr, s.tlsConfig) | |||
if s.tlsConfig != nil && s.network == "tcp" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe a constant for "tcp" would be ideal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll open a new pull request to change tcp/udp strings into constants, we use these strings in several places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! minor comment on tcp/udp strings
3619478
to
0c38e5d
Compare
do not load TLS configuration for UDP Signed-off-by: Katarzyna Kujawa <kkujawa@sumologic.com>
0c38e5d
to
7d8072b
Compare
**Description:** fix setting network connection, do not load TLS configuration for UDP **Link to tracking Issue:** open-telemetry#31130 **Testing:** unit test, manual tests with syslog server **Documentation:** added information that TLS config is applied only when TCP is used Signed-off-by: Katarzyna Kujawa <kkujawa@sumologic.com>
use consts for network protocol names, changes were suggested in #31202 (comment) **Testing:** unit tests, manual tests with syslog server --------- Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Description: fix setting network connection, do not load TLS configuration for UDP
Link to tracking Issue: #31130
Testing: unit test, manual tests with syslog server
Documentation: added information that TLS config is applied only when TCP is used