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

feat(registry): add sub_registry_with_labels #145

Merged
merged 1 commit into from Jul 10, 2023

Conversation

popadi
Copy link
Contributor

@popadi popadi commented May 28, 2023

@popadi popadi force-pushed the multiple-labels-sub-registry branch from 89f8c3c to c0cfad7 Compare May 28, 2023 13:59
@popadi popadi force-pushed the multiple-labels-sub-registry branch 3 times, most recently from 68a67fd to d42bf63 Compare June 5, 2023 04:50
@popadi
Copy link
Contributor Author

popadi commented Jul 5, 2023

@mxinden any update/feedback on this?

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Change looks good to me. Thanks. Though now that we have sub_registry_with_labelS we can simply sub_registry_with_label, right? What do you think?

modified   src/registry.rs
@@ -239,11 +239,20 @@ impl Registry {
         &mut self,
         label: (Cow<'static, str>, Cow<'static, str>),
     ) -> &mut Self {
-        let mut labels = self.labels.clone();
-        labels.push(label);
+        self.sub_registry_with_labels(std::iter::once(label))
+    }
+
+    /// Like [`Registry::sub_registry_with_prefix`] but with multiple labels instead.
+    pub fn sub_registry_with_labels(
+        &mut self,
+        labels: impl Iterator<Item = (Cow<'static, str>, Cow<'static, str>)>,
+    ) -> &mut Self {
+        let mut new_labels = self.labels.clone();
+        new_labels.extend(labels);
+
         let sub_registry = Registry {
             prefix: self.prefix.clone(),
-            labels,
+            labels: new_labels,
             ..Default::default()
         };

@popadi popadi force-pushed the multiple-labels-sub-registry branch from d42bf63 to dd0acd3 Compare July 8, 2023 14:16
@popadi
Copy link
Contributor Author

popadi commented Jul 8, 2023

Change looks good to me. Thanks. Though now that we have sub_registry_with_labelS we can simply sub_registry_with_label, right? What do you think?

Thanks for the suggestion! I updated the code.
Tests might need some extra work (I think individual tests per method would also be useful, alongside the existing one), but I will probably do that in a separate pull request.

@popadi popadi force-pushed the multiple-labels-sub-registry branch 2 times, most recently from 26f83c6 to 23f651d Compare July 8, 2023 14:19
Signed-off-by: Adrian Pop <popadrian1996@gmail.com>
@popadi popadi force-pushed the multiple-labels-sub-registry branch from 23f651d to 03bbfbb Compare July 8, 2023 14:21
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Thanks for the follow-ups.

@mxinden mxinden changed the title Fix #144: Add sub_registry_with_labels method to Registry feat(registry): add sub_registry_with_labels Jul 10, 2023
@mxinden mxinden merged commit 6c3c2f0 into prometheus:master Jul 10, 2023
11 checks passed
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.

None yet

2 participants