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.

Sign up for GitHub

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

Jump to bottom

Prototype using our own embedded HTTP server #3780

Closed
wants to merge 3 commits into from
Closed

Prototype using our own embedded HTTP server #3780

wants to merge 3 commits into from

Conversation

thampiotr
Copy link
Contributor

@thampiotr thampiotr commented May 4, 2023 •
edited
Loading

PR Description

Problem

Thinking about our recent components that embed an HTTP server, there are a few issues with weaveworks sever:

  • Starts GRPC server always which can lead to port conflicts and will use more resources. We only require HTTP embedded servers currently.
  • Has issues with re-registering existing metrics when we want to restart it - which requires workarounds: loki.source.heroku: use an unchecked collector for server metrics #3711
  • Requires adapter code for converting configs from River to weaveworks, which is a bit boilerplate (even though the shared river configs are much nicer than what we had before).

Proposed solution

The weaveworks server is not very complex though (500LOC) and I think we can maintain a similar implementation of our own that would be simpler and suit our needs better.

This PR is a quick hack to show how this could work in practice and verify how much code would this require.

Alternatives

Alternatives are:

  • Keep working around weavework server's issues, which I feel results in more boilerplate
  • Contribute upstream so we can e.g. disable GRPC and re-register metrics as needed - I suspect this wouldn't be desirable for the upstream, since if someone wants just HTTP, why not just use standard built-in which would make things simpler?

Which issue(s) this PR fixes

Notes to the Reviewer

PR Checklist

  • Fix invalid comments
  • Clean up the prototype
  • CHANGELOG updated
  • Documentation added
  • Tests updated

thampiotr
thampiotr commented May 4, 2023
View reviewed changes
Copy link
Contributor Author

@thampiotr thampiotr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments to help read the code.

component/common/net/config.go
Comment on lines +29 to +30
// ==== configuration NOT exposed in River configs ===

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exposing these in River would be trivial now and all components could benefit from new configuration options, e.g. TLS.

component/common/net/config.go
)

// ServerConfig is a River configuration that allows one to configure a weaveworks.Server. It
// exposes a subset of the available configurations.
// ServerConfig for a Server
type ServerConfig struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This struct is a merge of our ServerConfig and the weaveworks' server Config.
But we would be in control of how this works and what is available in River.

component/common/net/tls_config.go
@@ -0,0 +1,55 @@
package net
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is verbatim copied from weaveworks common.

component/common/net/server.go
@@ -1,97 +1,353 @@
package net
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a modified copy of weaveworks' common server. I've removed the GRPC support and made it work with our merged configuration struct.

component/common/net/server.go
Name: "tcp_connections",
Help: "Current number of accepted TCP connections.",
}, []string{"protocol"})
reg.MustRegister(tcpConnections)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will now be able to use Register and reuse the existing metric if it's already registered. Avoiding the need for workaround here #3711

@thampiotr
Copy link
Contributor Author

@thepalbi @tpaschalis When you have some time, I would appreciate some feedback on this idea.
(note, this is not ready for merge, just to demonstrate how this could work)

@thampiotr
Copy link
Contributor Author

@thepalbi @tpaschalis When you have some time, I would appreciate some feedback on this idea. (note, this is not ready for merge, just to demonstrate how this could work)

Had a chat with @tpaschalis about this and we'll explore a couple of alternatives before that, but we keep it as a an open alternative for the future.

We should note that we already have a fork of weaveworks server in pkg/server/server.go and maybe these could be unified?

@tpaschalis tpaschalis mentioned this pull request May 8, 2023
Merged
4 tasks
@thampiotr thampiotr closed this Dec 11, 2023
@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Feb 21, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@thampiotr