Skip to content
This repository was archived by the owner on Oct 25, 2023. It is now read-only.

Skip partition labels #33

Merged
merged 3 commits into from
May 20, 2020
Merged

Skip partition labels #33

merged 3 commits into from
May 20, 2020

Conversation

cotedm
Copy link
Contributor

@cotedm cotedm commented May 19, 2020

Skip partition label group by per #31

Skip partition label group by per Dabz#31
for _, label := range metric.Labels {
groupBy = append(groupBy, "metric.label."+label.Key)
}
groupBy := []string{}
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation seems off.

Copy link
Owner

Choose a reason for hiding this comment

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

Got the same feeling, go fmt seems right, but: @cotedm used spaces instead of tab.
Debating about space vs tabs is not really productive, but I will change those to tabs in order to be consistent with the rest of the files.

Copy link
Owner

Choose a reason for hiding this comment

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

indentation: fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops sorry about the format mismatch!

@Dabz Dabz linked an issue May 20, 2020 that may be closed by this pull request
@Dabz
Copy link
Owner

Dabz commented May 20, 2020

I tested locally, it seems to have no impact (one day - I will have automatic testing ^^). Let me merge and it should trigger the docker build automatically

@Dabz
Copy link
Owner

Dabz commented May 20, 2020

Actually, I noticed that we still return the partition label to Prometheus:
ccloud_metric_received_bytes{cluster_id="lkc-ldrq7",partition="<nil>",topic="_confluent-controlcenter-5-5-0-1-KSTREAM-OUTEROTHER-0000000105-store-changelog"}
I will still merge it, in a later release I will ensure that this label is not pushed to Prometheus.

@Dabz Dabz merged commit 2cb7620 into Dabz:master May 20, 2020
@sirianni
Copy link
Contributor

sirianni commented May 21, 2020

Late to the party here, but would it be safer to have a whitelist of labels that we include rather than a blacklist (partition being on the "blacklist"). This was the original implementation that @Dabz had prior to this change

Ideally this would be configurable. This is how the cloudwatch exporter works (see aws_dimension config).

@cotedm cotedm deleted the patch-2 branch May 21, 2020 19:37
@Dabz
Copy link
Owner

Dabz commented May 22, 2020

Completely agree with you @sirianni , I merged it as a "quick win" but I am planning to do some refactoring in the near future (aka, once I got a free day). I opened #31 and #32 to trace my plans (and mostly not to forget them 😅 ). The goal is:

  • Having a whitelist of labels we want to group by and avoid any hardcoded value in the functions itself
  • Do not push all labels to Prometheus, but only the one we are grouping by (otherwise we got null values)
  • Having a configurable whitelist of metrics we want to expose (either through a CLI or configuration file)

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.

Remove grouping per partition
4 participants