Skip to content

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

Closed
izeye wants to merge 2 commits intospring-projects:mainfrom
izeye:buffered
Closed

Configuration properties for Statsd's buffered and step properties are missing#30898
izeye wants to merge 2 commits intospring-projects:mainfrom
izeye:buffered

Conversation

@izeye
Copy link
Copy Markdown
Contributor

@izeye izeye commented May 8, 2022

This PR changes to expose buffered property for StatsdConfig.

See micrometer-metrics/micrometer#1375

@wilkinsona
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

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

@wilkinsona
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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