Skip to content

Configuration properties for Statsd's buffered and step properties are missing #30898

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

Closed
wants to merge 2 commits into from

Conversation

izeye
Copy link
Contributor

@izeye izeye commented May 8, 2022

This PR changes to expose buffered property for StatsdConfig.

See micrometer-metrics/micrometer#1375

@wilkinsona
Copy link
Member

wilkinsona commented May 8, 2022

Thanks, @izeye. It's a shame that we missed this. I've opened #30901 to see if we can stop if from happening again.

There is a workaround for the missing property, but it's not very elegant:

@Bean
StatsdConfig statsdConfig(StatsdProperties statsdProperties) {
    return new StatsdPropertiesConfigAdapter(statsdProperties) {

        @Override
        public boolean buffered() {
            return false;
        }

    };
}

As such, I am a bit tempted to treat this as a bug and fix it in 2.5.x. Flagging for team attention to see what everyone else thinks.

@wilkinsona wilkinsona added the for: team-attention An issue we'd like other members of the team to review label May 8, 2022
@scottfrederick
Copy link
Contributor

I think it's reasonable to consider this a bug of omission.

@wilkinsona wilkinsona changed the title Expose buffered property for StatsdConfig Configuration property for Statsd's buffered property is missing May 12, 2022
@wilkinsona wilkinsona added type: bug A general bug and removed for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged labels May 12, 2022
@wilkinsona wilkinsona added this to the 2.5.x milestone May 12, 2022
@wilkinsona
Copy link
Member

I've started looking at implementing something for #30901 and it looks like we're missing a configuration property for step as well. WDYT, @izeye? Does it make sense to have a configuration property for it?

@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label May 12, 2022
@izeye
Copy link
Contributor Author

izeye commented May 12, 2022

@wilkinsona I don't have much experience working with StatsD, but it seems to have been missed unintentionally, too and it sounds reasonable to align configurability level with its corresponding meter registry configuration.

/cc @shakuzen

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels May 12, 2022
@shakuzen
Copy link
Member

Statsd's step makes sense to expose as a configuration property and I suspect we just missed it.

@wilkinsona
Copy link
Member

Thanks, @shakuzen. @izeye, do you have time to update this PR to add step as well?

@wilkinsona wilkinsona added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels May 16, 2022
@izeye
Copy link
Contributor Author

izeye commented May 16, 2022

@wilkinsona Sure, I pushed ccbe758 for it.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels May 16, 2022
@snicoll snicoll self-assigned this May 16, 2022
@snicoll snicoll changed the title Configuration property for Statsd's buffered property is missing Configuration property for Statsd's buffered and step properties are missing May 16, 2022
snicoll pushed a commit that referenced this pull request May 16, 2022
@snicoll snicoll closed this in e703acb May 16, 2022
@snicoll
Copy link
Member

snicoll commented May 16, 2022

Thanks again @izeye

@wilkinsona wilkinsona changed the title Configuration property for Statsd's buffered and step properties are missing Configuration properties for Statsd's buffered and step properties are missing May 16, 2022
@wilkinsona wilkinsona modified the milestones: 2.5.x, 2.5.14 May 16, 2022
@izeye izeye deleted the buffered branch May 16, 2022 15:01
@wilkinsona wilkinsona removed the status: feedback-provided Feedback has been provided label May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants