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

Create goBackwardCompatibleMetrics option that defaults to true #226

Open
ktrz opened this issue Feb 1, 2024 · 4 comments
Open

Create goBackwardCompatibleMetrics option that defaults to true #226

ktrz opened this issue Feb 1, 2024 · 4 comments

Comments

@ktrz
Copy link
Member

ktrz commented Feb 1, 2024

As I was fixing #224 I've noticed that for Go benchmarks that output multiple metrics the action prior to v1.18.0 was extracting a unit that was slightly wrong. For this reason to keep backward compatibility I've kept this metric (PR #225) being calculated like that (in addition to the new way with multiple metric)

We should create an option that would default to keep this backward compatibility but would enable opting out of this behavior. In the next version it would get deprecated with a warning and then removed with only one way of extracting metrics

@ktrz ktrz changed the title Make goBackwardCompatibleMetrics option that defaults to true Create goBackwardCompatibleMetrics option that defaults to true Feb 1, 2024
@ningziwen
Copy link
Member

ningziwen commented Feb 2, 2024

Just want to clarify: when goBackwardCompatibleMetrics is true, does it mean it will return to the old wrong approach, outputting single unit?

@ktrz
Copy link
Member Author

ktrz commented Feb 2, 2024

My intention was to emit both old approach and new approach in case when goBackwardCompatibleMetrics is true. This way we make sure that existing users will still get the same data for their existing past benchmarks as well as the new data split by metric

@ktrz
Copy link
Member Author

ktrz commented Feb 2, 2024

In #225 I've implemented the backward compatibility but in this issue I want to make that optional (with backward compatibility by default)

@ningziwen
Copy link
Member

I don't have strong preference but adding the option seems not a normal approach to me. Too much overhead to maintain it.

Current state is good enough to me. We can just mark it deprecated and remove the support when we bump major version at some point.

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

No branches or pull requests

2 participants