Skip to content
This repository has been archived by the owner on Mar 17, 2024. It is now read-only.

Adding support to control which prometheus metrics to expose #62

Merged
merged 2 commits into from
Sep 16, 2019

Conversation

khorshuheng
Copy link
Contributor

This PR addresses: #43

New configuration exposed: metric-whitelist. PrometheusEndpointSink will report or remove metrics based on the list of regex.

This PR also added test cases for PrometheusEndpointSink, and fix a bug where metrics removal does not consider global tags.

@seglo seglo self-requested a review September 15, 2019 15:23
Copy link
Owner

@seglo seglo left a comment

Choose a reason for hiding this comment

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

Great work @khorshuheng. Thanks for adding the test for PrometheusEndpointSink. It would be nice if the remove logic can be cleaned up, but it's not necessary. Let me know what you think.

def clustersGlobalLabels(): ClusterGlobalLabels = {
clusters.map { cluster =>
cluster.name -> cluster.labels
}.toMap
Copy link
Owner

Choose a reason for hiding this comment

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

Nice cleanup.

metrics.foreach { case (_, gaugeDefinitionsForCluster) =>
val globalLabelValuesForCluster = clusterGlobalLabels.getOrElse(m.clusterName, Map.empty)
gaugeDefinitionsForCluster.get(m.definition).foreach(_.remove(globalLabelValuesForCluster.values.toSeq ++ m.labels: _*))
}
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for fixing this.

[minor] This could be made more clear with a for expression.

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 have substituted foreach with for expression. Let me know if the changes is what you have in mind.

@seglo
Copy link
Owner

seglo commented Sep 16, 2019

LGTM. Thanks @khorshuheng.

@seglo seglo merged commit 46648d3 into seglo:master Sep 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants