-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
fix(outputs.wavefront): update wavefront sdk and use non-deprecated APIs #11560
fix(outputs.wavefront): update wavefront sdk and use non-deprecated APIs #11560
Conversation
24addb3
to
b2f3822
Compare
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.
Thank you for taking the time to put up this PR!
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.
Thanks @LukeWinikates for the fix. The code looks good, I only would like to see the two new functions to not be exported. Can you please change this?
I've learned a couple of things about the wavefront SDK deprecations. It might make sense to open an issue for discussing. It turns out that you can't create a TCP connection in the Wavefront Go SDK unless you use the Unfortunately that means my PR needs to be updated, and we have to choose between continuing to use the deprecated functions and introducing a behavior change. I wonder if folks actually successfully use the TCP connection for the Wavefront output plugin. When my team first tried it, we found that it eventually dies silently because of socket timeouts. #7160 describes the same scenario. For that reason, we've always used the http connection. I can see a few different options here: One is to close this PR, and keep using the deprecated APIs Another would be to make the I'm going to update this PR to follow the latter path - I'd love to know what y'all think would be best here. |
Pushing what I have at the moment - I will also make some documentation updates tomorrow. |
1781585
to
1297d0b
Compare
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.
Looks good to me. Thanks @LukeWinikates for the fix!
Regarding the usage of deprecated functions vs. removal of TCP... I'd say let's first merge this PR as it is better than nothing. Then open an issue and we can discuss on how to handle the issue. It would be nice to know the timeframe for the removal of deprecated functions, maybe we need a new version of the plugin... But let's discuss this in the issue.
932d285
to
2113e1c
Compare
For the deprecation, can you please annotate the options similar to |
2113e1c
to
8661c9b
Compare
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.
Thanks again for driving these changes!
@LukeWinikates can you please rebase your PR on the latest master to solve the merge conflicts!? |
8661c9b
to
f320db8
Compare
- always create http(s) connection
…or and deprecated fields
f320db8
to
7b2efff
Compare
Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
Required for all PRs
The Wavefront output plugin uses APIs from the Wavefront Go SDK that have been deprecated. This PR updates the SDK version and adopts the currently recommended APIs for configuring the Wavefront sender.