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

Vendor update weaveworks/common #4922

Merged
merged 8 commits into from
May 10, 2023

Conversation

krajorama
Copy link
Contributor

@krajorama krajorama commented May 4, 2023

What this PR does

Update to weaveworks/common@102db1b5bbb9

This prepares exposing the histogram metric family cortex_request_duration_seconds as native histogram on GRPC.

Note that this also aligns the weaveworks common and mimir grpc versions again.

Which issue(s) this PR fixes or relates to

Related to #4175

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Update to weaveworks/common@4897cec

This prepares exposing Mimir the histogram metric family
cortex_request_duration_seconds as native histogram on GRPC.

Note that this also aligns the weaveworks common and mimir grpc versions
again.

Signed-off-by: György Krajcsovits <[email protected]>
@krajorama krajorama requested a review from a team as a code owner May 4, 2023 08:20
@krajorama krajorama marked this pull request as draft May 4, 2023 08:20
Signed-off-by: György Krajcsovits <[email protected]>
Copy link
Contributor

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

Needs CHANGELOG; some small notes below.

@@ -208,7 +209,7 @@ require (
github.com/rs/cors v1.9.0 // indirect
github.com/rs/xid v1.4.0 // indirect
github.com/sean-/seed v0.0.0-20170313163322-e2103e2c3529 // indirect
github.com/sercand/kuberesolver v2.4.0+incompatible // indirect
github.com/sercand/kuberesolver/v4 v4.0.0 // indirect
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's worth calling out this change, which was required by the gRPC update, just in case it works differently.

Copy link
Member

Choose a reason for hiding this comment

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

Also added to changelog.

@@ -1260,8 +1265,8 @@ google.golang.org/genproto/googleapis/rpc/errdetails
google.golang.org/genproto/googleapis/rpc/status
google.golang.org/genproto/googleapis/type/date
google.golang.org/genproto/googleapis/type/expr
# google.golang.org/grpc v1.53.0 => google.golang.org/grpc v1.47.0
## explicit; go 1.14
# google.golang.org/grpc v1.53.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest noting the from/to version numbers in changelog.

Copy link
Member

Choose a reason for hiding this comment

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

Also added to changelog.

# (advanced) Optionally log requests at info level instead of debug level.
# CLI flag: -server.log-request-at-info-level-enabled
[log_request_at_info_level_enabled: <boolean> | default = false]

[log_request_exclude_headers_list: <string> | default = ""]
Copy link
Contributor

Choose a reason for hiding this comment

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

could go in changelog; some users will like the ability to restrict more headers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm adding a PR in weaveworks common to add the flag description as well.

Copy link
Member

Choose a reason for hiding this comment

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

Added to changelog.

@@ -564,10 +564,14 @@ grpc_tls_config:
# CLI flag: -server.log-source-ips-regex
[log_source_ips_regex: <string> | default = ""]

[log_request_headers: <boolean> | default = ]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure I didn't intend for this to be user-visible, but here we are.

@pstibrany pstibrany marked this pull request as ready for review May 9, 2023 14:40
@pstibrany pstibrany requested a review from a team as a code owner May 9, 2023 14:40
Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Approving, but I also did some changes in the PR.

I could use this in #4956.

@pstibrany
Copy link
Member

pstibrany commented May 10, 2023

Merging to unblock myself. Happy to address post-merge feedback.

@pstibrany pstibrany merged commit f3afcb7 into main May 10, 2023
@pstibrany pstibrany deleted the krajo/20230504-native-cortex-requests-seconds branch May 10, 2023 07:29
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.

3 participants