-
Notifications
You must be signed in to change notification settings - Fork 526
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
Upgrade to latest dskit and weaveworks/common #2309
Conversation
3b64850
to
6679076
Compare
ExcludeRequestInLog bool `yaml:"-"` | ||
RegisterInstrumentation bool `yaml:"register_instrumentation"` | ||
ExcludeRequestInLog bool `yaml:"-"` | ||
DisableRequestSuccessLog bool `yaml:"-"` |
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.
We want to disable this in Mimir. We can do it in the same place where we set ExcludeRequestInLog
too:
https://github.com/grafana/mimir/blob/main/pkg/mimir/mimir.go#L125
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 @pracucci, will look into it.
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.
@pracucci I've disabled it.
docs/sources/operators-guide/configuring/reference-configuration-parameters/index.md
Show resolved
Hide resolved
docs/sources/operators-guide/configuring/reference-configuration-parameters/index.md
Outdated
Show resolved
Hide resolved
f4d8f82
to
693fc03
Compare
After discussing with @pracucci, we've agreed I'm going to make a PR to weaveworks/common to hide new configuration parameters from its dependency. |
PR to weaveworks/common made. |
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
693fc03
to
dad8218
Compare
I've upgraded to the latest weaveworks/common, which removes the new TLS config parameters. |
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.
LGTM! I don't feel strong having -server.log-request-at-info-level-enabled
as basic instead of advanced.
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
@pracucci I added a couple of changelog entries from dskit. Should I keep them? |
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.
LGTM, thanks!
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
* Upgrade to latest dskit and weaveworks/common * Fix flaky tests Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
What this PR does
Upgrade to latest dskit and weaveworks/common, where
dskit/middleware.PrometheusGRPCUnaryInstrumentation
moves toweaveworks/common/middleware.UnaryClientInstrumentInterceptor
.Which issue(s) this PR fixes or relates to
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]