Skip to content

Conversation

@pstibrany
Copy link
Contributor

What this PR does:

This PR fixes registration of memberlist_client_kv_store_count metric (this broke in #22) and also disables expiration of internal memberlist-library metrics.

This PR also removes MetricsRegisterer field from memberlist.KVConfig struct in favor of registrerer passed to memberlist.NewKV function.

Which issue(s) this PR fixes:

Fixes #305

Checklist

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

…s registrerer is passed as argument to NewKV.

Fix registration of `memberlist_client_kv_store_count` metric.
@pracucci pracucci self-requested a review June 27, 2023 14:18
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

LGTM. I checked if in Mimir we passed a different registerer (e.g. maybe with different const labels, or so...) between NewKV() and KVConfig.MetricsRegisterer but looks like we don't. I suggest you to do a quick check to Loki and Tempo too before merging tho.

@pstibrany
Copy link
Contributor Author

LGTM. I checked if in Mimir we passed a different registerer (e.g. maybe with different const labels, or so...) between NewKV() and KVConfig.MetricsRegisterer but looks like we don't. I suggest you to do a quick check to Loki and Tempo too before merging tho.

Loki uses the same reg for both: NewKVInitService and MetricsRegisterer.

@pstibrany
Copy link
Contributor Author

LGTM. I checked if in Mimir we passed a different registerer (e.g. maybe with different const labels, or so...) between NewKV() and KVConfig.MetricsRegisterer but looks like we don't. I suggest you to do a quick check to Loki and Tempo too before merging tho.

Same is true in Tempo: NewKVInitService and MetricsRegisterer both use the same reg (prometheus.DefaultRegisterer).

@pstibrany pstibrany merged commit f335545 into main Jun 29, 2023
@pstibrany pstibrany deleted the metrics-registrerer branch June 29, 2023 14:42
@pstibrany pstibrany mentioned this pull request Jun 29, 2023
1 task
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.

Potential memberlist metrics issues.

2 participants