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

Add and use server package #41

Merged
merged 9 commits into from
Apr 6, 2022
Merged

Add and use server package #41

merged 9 commits into from
Apr 6, 2022

Conversation

carrieedwards
Copy link
Contributor

This PR adds and uses a server package to handle the requests.

@grafanabot

This comment has been minimized.

@grafanabot

This comment has been minimized.

@grafanabot

This comment has been minimized.

Copy link
Contributor

@ywwg ywwg left a comment

Choose a reason for hiding this comment

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

mostly some cleanup we need to do

pkg/route/registerer.go Outdated Show resolved Hide resolved
pkg/route/registerer.go Outdated Show resolved Hide resolved
pkg/server/middleware/logging.go Outdated Show resolved Hide resolved
pkg/server/middleware/logging_test.go Outdated Show resolved Hide resolved
pkg/server/middleware/logging_test.go Outdated Show resolved Hide resolved
pkg/server/server_test.go Outdated Show resolved Hide resolved
pkg/util/bytereplacer/byte_replacer_test.go Outdated Show resolved Hide resolved
@grafanabot

This comment has been minimized.

@ywwg
Copy link
Contributor

ywwg commented Apr 5, 2022

lint report has some items

ywwg
ywwg previously approved these changes Apr 5, 2022
Copy link
Contributor

@ywwg ywwg left a comment

Choose a reason for hiding this comment

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

very nice! Just one minor nit

pkg/influx/api.go Outdated Show resolved Hide resolved
@grafanabot

This comment has been minimized.

@carrieedwards
Copy link
Contributor Author

For the linting report, is it necessary to check the error value of the logger?

@leizor
Copy link
Contributor

leizor commented Apr 5, 2022

For the linting report, is it necessary to check the error value of the logger?

I've been prepending those lines with _ = (basically throwing away the result still, but doing it explicitly so the linter is happy).

@ywwg
Copy link
Contributor

ywwg commented Apr 5, 2022

I wonder if we can tweak the linter to ignore log return values specifically without having to do either this or a //nolint comment? This is annoying to have to do. There's an exhausting discussion here: go-kit/kit#164

@ywwg
Copy link
Contributor

ywwg commented Apr 5, 2022

"""
If, when using Go kit, you want to ensure all logging errors are handled in some way then I believe the correct approach is to write a Logger decorator that handles errors returned by a wrapped logger as desired.
"""

@ywwg
Copy link
Contributor

ywwg commented Apr 5, 2022

so it seems that we should create an adapter layer that deals with (ignores) the errors so we don't have to hack the call site to satisfy the linter. Example: go-kit/kit#164 (comment)

@carrieedwards
Copy link
Contributor Author

Should I try to fix the linting in a separate PR?

@ywwg
Copy link
Contributor

ywwg commented Apr 6, 2022

you should at least fix the dead code lint, but yeah we can address the logging lint later

@ywwg
Copy link
Contributor

ywwg commented Apr 6, 2022

and you do want to check the value of server.Run, like:

go func() {
if err := server.Run() ;err != nil {...}
}()

@grafanabot

This comment has been minimized.

@grafanabot
Copy link

Go coverage report:

Click to expand.
File %
github.com/grafana/influx2cortex/pkg/errorx/errors.go 67.3%
github.com/grafana/influx2cortex/pkg/errorx/http_translator.go 100.0%
github.com/grafana/influx2cortex/pkg/influx/api.go 90.0%
github.com/grafana/influx2cortex/pkg/influx/errors.go 100.0%
github.com/grafana/influx2cortex/pkg/influx/parser.go 84.1%
github.com/grafana/influx2cortex/pkg/influx/recorder.go 91.7%
github.com/grafana/influx2cortex/pkg/remotewrite/client.go 69.1%
github.com/grafana/influx2cortex/pkg/remotewrite/measured_client.go 88.9%
github.com/grafana/influx2cortex/pkg/remotewrite/recorder.go 100.0%
github.com/grafana/influx2cortex/pkg/route/registerer.go 100.0%
github.com/grafana/influx2cortex/pkg/server/middleware/http_auth.go 0.0%
github.com/grafana/influx2cortex/pkg/server/middleware/http_fake_auth.go 0.0%
github.com/grafana/influx2cortex/pkg/server/middleware/instrument.go 6.7%
github.com/grafana/influx2cortex/pkg/server/middleware/logging.go 69.7%
github.com/grafana/influx2cortex/pkg/server/middleware/middleware.go 0.0%
github.com/grafana/influx2cortex/pkg/server/middleware/request_limits.go 81.8%
github.com/grafana/influx2cortex/pkg/server/middleware/response.go 57.1%
github.com/grafana/influx2cortex/pkg/server/middleware/route_matcher.go 0.0%
github.com/grafana/influx2cortex/pkg/server/middleware/tracer.go 13.0%
github.com/grafana/influx2cortex/pkg/server/server.go 83.3%
github.com/grafana/influx2cortex/pkg/tenant/resolver.go 75.6%
github.com/grafana/influx2cortex/pkg/tenant/tenant.go 91.7%
github.com/grafana/influx2cortex/pkg/util/bytereplacer/byte_replacer.go 100.0%
github.com/grafana/influx2cortex/pkg/util/extract_forwarded.go 66.7%
github.com/grafana/influx2cortex/pkg/util/http.go 68.3%
github.com/grafana/influx2cortex/pkg/util/log/experimental.go 0.0%
github.com/grafana/influx2cortex/pkg/util/log/log.go 0.0%
github.com/grafana/influx2cortex/pkg/util/log/rate_limit.go 90.0%
github.com/grafana/influx2cortex/pkg/util/log/wrappers.go 0.0%
github.com/grafana/influx2cortex/pkg/util/push/push.go 60.0%
total 65.5%

Go lint report:

Click to expand.
pkg/server/server.go:155:24: Error return value of `(github.com/go-kit/log.Logger).Log` is not checked (errcheck)
		level.Info(s.log).Log("msg", "Starting http server", "addr", s.httpListener.Addr().String())
		                     ^
pkg/server/server.go:165:24: Error return value of `(github.com/go-kit/log.Logger).Log` is not checked (errcheck)
		level.Info(s.log).Log("msg", "Starting grpc server", "addr", s.grpcListener.Addr().String())
		                     ^
pkg/server/server.go:182:23: Error return value of `(github.com/go-kit/log.Logger).Log` is not checked (errcheck)
	level.Info(s.log).Log("msg", "Shutting down http server")
	                     ^

Copy link
Contributor

@ywwg ywwg left a comment

Choose a reason for hiding this comment

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

thanks! good work importing this raft of packages

@carrieedwards carrieedwards merged commit 110df72 into main Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants