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

Introduce 'default tags' to the dogstatsd payload #752

Closed
wants to merge 2 commits into from

Conversation

blt
Copy link
Collaborator

@blt blt commented Nov 29, 2023

What does this PR do?

This commit allows the user to specify a list of default tags to prefix on any generated tagset. These tags are assumed to be in valid form -- key:value,key:value -- without a trailing comma. Users may pass an empty string.

Related issues

REF SMPTNG-91

This commit allows the user to specify a list of default tags to prefix on any
generated tagset. These tags are assumed to be in valid form --
key:value,key:value -- without a trailing comma. Users may pass an empty string.

REF SMPTNG-91

Signed-off-by: Brian L. Troutwine <[email protected]>
@blt blt requested a review from a team as a code owner November 29, 2023 20:03
Signed-off-by: Brian L. Troutwine <[email protected]>
Comment on lines +295 to +298
/// Selection of tag pairs to always prepend to tagsets. Assumed to be
/// valid, do not count toward `tags_per_msg`.
#[serde(default = "default_tags")]
pub default_tags: Vec<String>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: this could be validated using serde's deserialize_with attribute https://serde.rs/field-attrs.html#deserialize_with

Comment on lines +67 to +69
if let Some(default_tag) = self
.default_tags
.choose(&mut *self.internal_rng.borrow_mut())
Copy link
Contributor

Choose a reason for hiding this comment

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

I may be misunderstanding how this works; this looks like it'd pick one tag to prepend. Is that correct? That behavior doesn't match the config doc string.

@scottopell
Copy link
Contributor

I don't think prepending a static set of tags will meaningfully change the relationship between # contexts and # unique tags, which is the goal of SMPTNG-91.

I don't have a super clear idea of how I want to do this, but something along these lines:

  1. Generate a "small" amount of tags to be the "common tags" (tbd what "small" means, maybe num_contexts / 100 ?)
  2. For each desired context, sample out of "common tags" until we reach a desired "common" amount (maybe num_tags / 2 ?)
  3. At this point, our metric template may or may not be a unique context (we could have chose the same "common tags" in step 2 for some other metric template). Fill out the rest of the tags with random & unique tags to get a unique context.

There is some uncertainty in there, but I think the two key distinctions I want to make is that the "common" tags are sampled from a pool (rather than being a static set) and that they consume from the "num_desired_tags" budget.

@blt
Copy link
Collaborator Author

blt commented Nov 30, 2023

Generate a "small" amount of tags to be the "common tags" (tbd what "small" means, maybe num_contexts / 100 ?)

I'd vote for a configurable value.

There is some uncertainty in there, but I think the two key distinctions I want to make is that the "common" tags are sampled from a pool (rather than being a static set) and that they consume from the "num_desired_tags" budget.

Gotcha. Okay, that would be a little more involved then. It's not a hard pull -- you have to make a pool as a part of the tag generator that's sampled from preferentially -- but this PR isn't that.

@scottopell
Copy link
Contributor

Gotcha. Okay, that would be a little more involved then. It's not a hard pull -- you have to make a pool as a part of the tag generator that's sampled from preferentially -- but this PR isn't that.

fair enough, if you'd like to land this as-is I don't object, I just don't think it fully addresses the gap described in smptng-91.

@scottopell
Copy link
Contributor

its still a WIP, but I've started to implement my ideas in #761

@blt
Copy link
Collaborator Author

blt commented Dec 5, 2023

Closing in favor of #761.

@blt blt closed this Dec 5, 2023
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.

3 participants