Migrate weaveworks/common packages used by Mimir #342
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Add gRPC streaming middleware. Adds a series of gRPC stream middleware for logging, user header propagation and tracing. This is laying groundwork for using gRPC streaming between ingesters and queriers in Cortex. * Update the vendored versions. * Pin kuberesolver because the API has changed. * Always put code under github.com/weaveworks/common; Dependancies between packages won't work if the code is checked out in github.com/tomwilkie, for instance. Also, common is a very common name for a repo, so its not uncommon for the repo to be called something like weaveworks-common. Signed-off-by: Tom Wilkie <[email protected]>
…sked (#102) * Don't enable trace sampling by default * Don't export new unused member, simplify docstring
* Move dedupe code to dedupe.go * Add unified logging interface. * Adapt middleware/, user/, signals/ and server/ to new logging interfaces. Allow user to specify logging implementation in server, if non is specified default to logrus. As part of this we need log level as server config, and for convenience we expose the server logger. * Update vendoring * Expose the various level types. Signed-off-by: Tom Wilkie <[email protected]>
gRPC behaviour should be the same as HTTP: Run() exits when a signal is received, and Stop() stops the handling of requests. And as we no longer call GracefulShutdown in Run, call it in Stop - one should always try to shutdown gracefully. Signed-off-by: Tom Wilkie <[email protected]>
* Propagate tracing context over httpgrpc * Log warnings for failed trace inject/extract
Random people on the Internet can send in arbitrary paths, so collapse them all into "other" when reporting metrics.
Primary motivation is to allow a standard Go profiling request to complete, for which I only need the write timeout, but I raised the other 5-second timeouts too since it isn't that unusual to have operations or data uploads take longer.
* Return errors when http/grpc servers exit. There was a case where the HTTP server died but the process exited. We need to exit when the servers exit. Signed-off-by: Goutham Veeramachaneni <[email protected]>
Signed-off-by: Goutham Veeramachaneni <[email protected]>
* Improve http request logging When logging an http request, e.g. on 500 error, put all the headers and response on one line. Also strip cookies and csrf tokens on security grounds. * Add a test to generate an http error to check formatting
* Refactor: extract functions HTTPRequest(), WriteResponse(), WriteError() * Regenerate httpgrpc.pb.go with protoc-gen-gogo from https://github.com/gogo/protobuf Change taken from cortexproject/cortex@f807690 where no explanation was given.
* Expose the HTTP server from the server struct. This allows users to inject HTTP requests and have the logged, instrumented and traced correctly. * Fix #120: buffer the errChan, as one of the servers could return an error before Run starts listening on errChan. Signed-off-by: Tom Wilkie <[email protected]>
Signed-off-by: Goutham Veeramachaneni <[email protected]>
* Refactor - s/logusEntry/logrusEntry * Also set global logging when initializing logrus This PR sets the global logging instance when calling `logging.Setup(level)`. Previously, only the standard logrus logger was set up and if anyone used the new funcs such as `logging.Info()` it was discarded.
* Don't trace http requests when using httpgrpc. * Demonstrate how the same can be acheived with middleware.Tracer.
* Print logrus output in logfmt format
This changes the way logrous outputs log messages in case there is no
TTY attached.
Changes from
ERRO: 2018/08/23 20:28:10.075952 Sample log message extra=foo
to
time="2018-08-23T20:28:10Z" level=info msg="Sample log message" extra="foo"
which follows the `logfmt` key-value format by also providing `time`,
`level`, and `msg` in adition to the extra fields.
The PR removes the custom `textFormatter` to have the default
`logrus.TextFormatter` in place which takes care of detecting whether a
terminal is attached or not and switches between text and logfmt output
automatically.
It appears the custom formatter was to prevent differences in rendering
depending on color/non-color output which I don't necessarily see as an
issue.
* dep ensure to fix tests
* Update 'dep' to 0.5.0 * Update more golang/protobuf to gogo/protobuf * add constraints for downstream users
…126) * Pass options to tracing middleware Signed-off-by: Goutham Veeramachaneni <[email protected]> * Use the route matcher instead Signed-off-by: Goutham Veeramachaneni <[email protected]>
* Update to latest kuberesolver and use a load balancing policy Signed-off-by: Goutham Veeramachaneni <[email protected]>
Signed-off-by: Tom Wilkie <[email protected]>
This can be used to serve an API from a prefix (e.g. /prometheus/) without having to re-write every HTTP Handler.
server: add PathPrefix to config for all routes
Signed-off-by: Tom Wilkie <[email protected]>
Cancellations are always caused by something else, so having them show up as errors in the tracing UI is distracting.
pracucci
approved these changes
Aug 3, 2023
bboreham
reviewed
Aug 3, 2023
| ) | ||
|
|
||
| // IsCanceled checks whether an error comes from an operation being canceled | ||
| func IsCanceled(err error) bool { |
There was a problem hiding this comment.
This is kinda duplicative of IsGRPCContextCanceled; do we need both?
There was a problem hiding this comment.
In the interests of keeping this migration as simple as possible, I'd like to keep both for now as they're not identical, but I'll come back and review this separately afterwards.
| package errors | ||
|
|
||
| // Error see https://dave.cheney.net/2016/04/07/constant-errors. | ||
| type Error string |
There was a problem hiding this comment.
I think this could be replaced by the Go standard library errors.New.
There was a problem hiding this comment.
In the interests of keeping this migration as simple as possible, I'd like to keep this for now, but I'll come back and review this separately afterwards.
charleskorn
added a commit
to grafana/mimir
that referenced
this pull request
Aug 4, 2023
1 task
charleskorn
added a commit
to grafana/loki
that referenced
this pull request
Aug 4, 2023
charleskorn
added a commit
to grafana/tempo
that referenced
this pull request
Aug 4, 2023
charleskorn
added a commit
to grafana/tempo
that referenced
this pull request
Aug 4, 2023
charleskorn
added a commit
to charleskorn/pyroscope
that referenced
this pull request
Aug 4, 2023
pracucci
pushed a commit
to grafana/mimir
that referenced
this pull request
Aug 4, 2023
simonswine
pushed a commit
to grafana/pyroscope
that referenced
this pull request
Aug 4, 2023
zalegrala
pushed a commit
to grafana/tempo
that referenced
this pull request
Aug 4, 2023
kavirajk
added a commit
to grafana/loki
that referenced
this pull request
Aug 7, 2023
**What this PR does / why we need it**: This PR upgrades dskit and replaces use of packages from weaveworks/common with their migrated equivalents in dskit. See grafana/dskit#342 for more details. Note that Loki still uses some packages from weaveworks/common that I haven't migrated (`aws` and `test`) - I'll migrate these separately. If this PR needs to be rebuilt, I used `rewrite.sh` ([source](https://gist.github.com/charleskorn/48efe62a09d6d70f3de30327003df5c5#file-rewrite-sh)) to generate most of these changes. **Which issue(s) this PR fixes**: (none) **Special notes for your reviewer**: (none) **Checklist** - [x] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [n/a] Documentation added - [n/a] Tests updated - [n/a] `CHANGELOG.md` updated - [n/a] If the change is worth mentioning in the release notes, add `add-to-release-notes` label - [n/a] Changes that require user attention or interaction to upgrade are documented in `docs/sources/setup/upgrade/_index.md` - [n/a] For Helm chart changes bump the Helm chart version in `production/helm/loki/Chart.yaml` and update `production/helm/loki/CHANGELOG.md` and `production/helm/loki/README.md`. [Example PR](d10549e) Co-authored-by: Kaviraj Kanagaraj <[email protected]>
This was referenced Aug 11, 2023
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does:
This PR moves all
github.com/weaveworks/commonpackages used by Mimir to this repo.We're migrating these packages because CI for
github.com/weaveworks/commonis broken formaster, and Weaveworks has expressed a desire to archive this repo.The following packages have been migrated, all to new
dskitpackages with the same name unless noted otherwise:github.com/weaveworks/common/errorstogithub.com/grafana/dskit/errorsgithub.com/weaveworks/common/grpctogithub.com/grafana/dskit/grpcutil(merged into existinggrpcutilpackage)github.com/weaveworks/common/httpgrpctogithub.com/grafana/dskit/httpgrpcgithub.com/weaveworks/common/instrumenttogithub.com/grafana/dskit/instrumentgithub.com/weaveworks/common/loggingtogithub.com/grafana/dskit/log(merged into existinglogpackage)github.com/weaveworks/common/middlewaretogithub.com/grafana/dskit/middlewaregithub.com/weaveworks/common/mtimetogithub.com/grafana/dskit/mtimegithub.com/weaveworks/common/servertogithub.com/grafana/dskit/servergithub.com/weaveworks/common/signalstogithub.com/grafana/dskit/signalsgithub.com/weaveworks/common/tracingtogithub.com/grafana/dskit/tracinggithub.com/weaveworks/common/usertogithub.com/grafana/dskit/userTempo depends only on the same packages. Loki also requires
awsandtest, which I'll migrate in a separate PR.Other changes of note are:
gogo/protobuf, rather than Google's implementation: 062ea8dTCPinstead ofTcp): bc7854cgolang.org/x/net/contextwithcontext: 2ea3a37prometheuswithpromauto, except ininstrumentpackage, where this would require changing the interface ofCollector,HistogramCollectorandJobCollector: 273c830If this migration needs to be repeated / redone in the future, these are the steps I followed:
Create
packages.txt(example) with source and destination package names separated by a tabI ran
go list -test -deps ./... | sort | grep -E '^github.com/weaveworks/common/[^/]*$' | sed -e "s#^github.com/weaveworks/common/##"in a working copy of Mimir to get top-level packages to moveRun
migrate.sh(source) to move the packages between repos, preserving history, rewriting file paths and adding provenance comments.Manually correct any compilation or linting issues in
dskitdirectory before committingWhich issue(s) this PR fixes:
(none)
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]