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

Chore: Upgrade weaveworks/common #4462

Merged
merged 4 commits into from
Sep 6, 2021
Merged

Chore: Upgrade weaveworks/common #4462

merged 4 commits into from
Sep 6, 2021

Conversation

aknuds1
Copy link
Contributor

@aknuds1 aknuds1 commented Sep 6, 2021 •
edited
Loading

What this PR does:
I suggest we upgrade weaveworks/common dependency to 1fa3f9fa874c.

The key changes in the new version of weaveworks/common are that instrument.Collector.Before and instrument.Collector.After now take a context argument and that ExtractTraceID was moved to the tracing package. Also, server.New takes a new configuration parameter cfg.GRPCListenNetwork.

I think the only change that could have any practical effect would be the switch to cfg.GRPCListenNetwork in server.New. I frankly don't know if that would affect Cortex.

Which issue(s) this PR fixes:

Checklist

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

@aknuds1
Chore: Upgrade weaveworks/common
a980baa 
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
@aknuds1 aknuds1 force-pushed the chore/upgrade-weaveworks-common branch from 7d5f2c3 to a980baa Compare September 6, 2021 07:55
@aknuds1 aknuds1 marked this pull request as ready for review September 6, 2021 07:56
pstibrany
pstibrany approved these changes Sep 6, 2021
View reviewed changes
Copy link
Contributor

@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.

Thank you.

vendor/github.com/weaveworks/common/middleware/instrument.go Show resolved Hide resolved
@bboreham
Copy link
Contributor

bboreham commented Sep 6, 2021

Please, when updating a dependency, can you list the key changes, or state that you looked and there are no key changes.

@aknuds1
Copy link
Contributor Author

aknuds1 commented Sep 6, 2021

Please, when updating a dependency, can you list the key changes, or state that you looked and there are no key changes.

@bboreham will do.

@aknuds1
Copy link
Contributor Author

aknuds1 commented Sep 6, 2021

@bboreham added key changes to PR description, PTAL.

@aknuds1
Add changelog entries
bfd5238 
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
@aknuds1
Copy link
Contributor Author

aknuds1 commented Sep 6, 2021

@bboreham added changelog entries, PTAL.

@aknuds1
Use instrument.NewHistogramCollector
a665ead 
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
@aknuds1
Use tracing.ExtractSampledTraceID
998a5de 
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
@pull-request-size pull-request-size bot added size/M and removed size/S labels Sep 6, 2021
@aknuds1
Copy link
Contributor Author

aknuds1 commented Sep 6, 2021

@bboreham switched to using weaveworks/common/instrument.HistogramCollector and weaveworks/common/tracing.ExtractSampledTraceID, make sense?

bboreham
bboreham approved these changes Sep 6, 2021
View reviewed changes
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.

lgtm

@bboreham
Copy link
Contributor

bboreham commented Sep 6, 2021

Yes, we can use instrument.HistogramCollector since the need for observableVecCollector went away due to changes in #2903, and ExtractSampledTraceID will avoid logging the trace ID if it has already been dropped by sampling, which makes for a better experience.

@pstibrany pstibrany merged commit 028c4b3 into cortexproject:master Sep 6, 2021
@aknuds1 aknuds1 deleted the chore/upgrade-weaveworks-common branch September 6, 2021 15:34
alvinlin123 pushed a commit to ac1214/cortex that referenced this pull request Jan 14, 2022
@aknuds1 @alvinlin123
Chore: Upgrade weaveworks/common (cortexproject#4462)
ed3589a 
* Chore: Upgrade weaveworks/common

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

* Add changelog entries

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

* Use instrument.NewHistogramCollector

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

* Use tracing.ExtractSampledTraceID

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Alvin Lin <alvinlin@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pstibrany pstibrany pstibrany approved these changes

@bboreham bboreham bboreham approved these changes

Assignees
No one assigned
Labels
size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@aknuds1 @bboreham @pstibrany