Skip to content
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

Healthchecks should use the same http client #2237

Closed
3 tasks done
lukesteensen opened this issue Apr 6, 2020 · 4 comments · Fixed by #2238
Closed
3 tasks done

Healthchecks should use the same http client #2237

lukesteensen opened this issue Apr 6, 2020 · 4 comments · Fixed by #2238
Assignees
Labels
domain: config Anything related to configuring Vector domain: networking Anything related to Vector's networking have: should We should have this feature, but is not required. It is medium priority. type: tech debt A code change that does not add user value.

Comments

@lukesteensen
Copy link
Member

lukesteensen commented Apr 6, 2020

Ref #2234

Currently, many of our healthchecks instantiate their own http client. This opens up the possibility for mismatched configs, etc.

We should go through each sink and ensure that, as much as possible, things like http clients are created in build and used for both the healthcheck and the sink itself.
For non-http-based sinks, we should at least double check that all connection settings are propogated correctly and look for ways to consolidate them.

Todo

  • Construct tcp healthcheck with TLS.
    • Affects sinks: vector, datadog, papertrail, tcp, socket.

  • Build HttpClient once.
    • All of our http sinks now depend on our util so this is actually quite straight forward.
    • Affects sinks: splunk_hec, loki, logdna, http, ...

  • Build aws clients once.
    • Affects all aws sinks.
@lukesteensen lukesteensen added type: bug A code related bug. type: tech debt A code change that does not add user value. domain: config Anything related to configuring Vector domain: networking Anything related to Vector's networking have: must We must have this feature, it is critical to the project's success. It is high priority. labels Apr 6, 2020
@bruceg
Copy link
Member

bruceg commented Apr 7, 2020

Did #2238 really close this? From my reading it only changed how TLS settings were handled, not the management of the HTTP client.

@LucioFranco
Copy link
Contributor

You're right #2238 doesn't directly solve this though, I do not think its as worth it. The problem for me lies more so with the complexness of building our tls types. I think we should ensure TlsSettings can only be built from the correct TlsConfig type instead of doing what was fixed in #2238.

From going through the sinks I have not really found it to be that helpful to build a single httpclient for both the sink and healthcheck. The things that build the httpclient are what we should ensure are the same.

That's mostly what I think but I'll reopen this since we may want to fix this in the future. But our current TLS issues are solved.

@LucioFranco LucioFranco reopened this Apr 7, 2020
@LucioFranco LucioFranco removed have: must We must have this feature, it is critical to the project's success. It is high priority. type: bug A code related bug. labels Apr 7, 2020
@LucioFranco LucioFranco removed their assignment Apr 7, 2020
@LucioFranco
Copy link
Contributor

So I am shifting this issue to something that is more so good to have rather than an actual bug since the bug this was originally connected has been solved in #2238.

@lukesteensen lukesteensen added the have: should We should have this feature, but is not required. It is medium priority. label Apr 7, 2020
@ktff
Copy link
Contributor

ktff commented Jul 14, 2020

Now all current sinks either have:

  • Client created once in/during build method
  • No healthcheck
  • Client like thing but requires await so it isn't practical to have it in build, but is created by a single method
  • Sink and healthcheck use different client like things. ex. kafka

@ktff ktff closed this as completed Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain: config Anything related to configuring Vector domain: networking Anything related to Vector's networking have: should We should have this feature, but is not required. It is medium priority. type: tech debt A code change that does not add user value.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants