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

[datadogexporter] Remove obsolete report_buckets config #5858

Merged

Conversation

KSerrania
Copy link
Contributor

@KSerrania KSerrania commented Oct 21, 2021

Description:

Removes the metrics::report_buckets option (the metrics::histograms::mode option should be used instead), which has been deprecated for two releases (v0.36.0 and v0.37.0).

Link to tracking Issue: n/a

Testing: Modified unit tests to fit the new situation.

Documentation: Modified inline docs & config examples.

@KSerrania KSerrania marked this pull request as ready for review October 21, 2021 13:45
@KSerrania KSerrania requested review from a team and dmitryax October 21, 2021 13:45
Comment on lines +325 to +327
// metrics::report_buckets is obsolete, return an error to
// tell the user to use metrics::histograms::mode instead.
if configMap.IsSet("metrics::report_buckets") {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this is necessary, an error will be thrown anyway because the field will not be recognized. Up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I decided to leave it in order to print the more specific error 'metrics::report_buckets' is obsolete. Use 'metrics::histograms::mode' instead so that users know what to do to fix it. We can probably remove it after a few releases.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, maybe add a tracking issue and assign to you or @mx-psi

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