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

Switch pkg/ingester to promauto.With(reg) #2299

Merged
merged 4 commits into from Mar 23, 2020

Conversation

pracucci
Copy link
Contributor

What this PR does:
Following the work related to #2204 to get rid of global metrics and registerer, in this PR I've migrated pkg/ingester metrics to promauto.With(registerer).

Few notes:

  • There's still a global metric in pkg/ingester/client which I will address in a separate PR
  • Not much happy with the path followed for the WAL and memorySeries. The problem is that the initial design doesn't look much clean in regards of global variables and I had to deal with that in order to keep the changeset small (but I admit there's much room for improvement design-wise there)

Which issue(s) this PR fixes:
N/A

Checklist

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

Copy link
Contributor

@codesome codesome left a comment

Choose a reason for hiding this comment

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

Not much familiar with the integration code yet, but other parts LGTM

pkg/ingester/transfer.go Outdated Show resolved Hide resolved
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.

Looks good to me, thanks for doing this cleanup.

pkg/ingester/metrics.go Outdated Show resolved Hide resolved
@pracucci pracucci force-pushed the switch-ingester-to-promauto branch from 44d7a54 to 366f2be Compare March 20, 2020 09:15
@gouthamve
Copy link
Contributor

gouthamve commented Mar 20, 2020

Not much happy with the path followed for the WAL and memorySeries.

Can you clarify this? Just curious as I didn't find anything obviously off when I glanced through the code. Is this about the integration tests or something about the WAL metrics themselves?

@pracucci
Copy link
Contributor Author

Can you clarify this? Just curious as I didn't find anything obviously off when I glanced through the code. Is this about the integration tests or something about the WAL metrics themselves?

@gouthamve I haven't been clear, sorry. My comment is about the change I did to pass the metric instances to some functions (batchSend(), newMemorySeries(), processCheckpointRecord()) because it looks a bad design to me. I'm not happy with my changes but I picked that path to avoid to do a larger refactoring and keeping the change set easier to diff and understand.

@pracucci pracucci force-pushed the switch-ingester-to-promauto branch from 9a9594f to 0b672f0 Compare March 20, 2020 17:10
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
…total metric

Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
@pracucci pracucci force-pushed the switch-ingester-to-promauto branch from 0b672f0 to 50694d1 Compare March 23, 2020 07:46
@pracucci pracucci merged commit fab8e34 into cortexproject:master Mar 23, 2020
@pracucci pracucci deleted the switch-ingester-to-promauto branch March 23, 2020 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants