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

remove formatter function from percentage diff evaluation #1347

Merged
merged 2 commits into from
Oct 4, 2021

Conversation

DJRickyB
Copy link
Contributor

@DJRickyB DJRickyB commented Oct 1, 2021

This commit fixes a small correctness issue with some metrics in the new Diff % column.

Before (sample):

|                                                        Metric |         Task |    Baseline |   Contender |         Diff |   Unit |     Diff % |
|--------------------------------------------------------------:|-------------:|------------:|------------:|-------------:|-------:|-----------:|
|                    Cumulative indexing time of primary shards |              |   0.0355667 |      0.0667 |      0.03113 |    min |      0.00% |
|             Max cumulative indexing time across primary shard |              |   0.0355667 |      0.0667 |      0.03113 |    min |      0.00% |
|                       Cumulative merge time of primary shards |              |     0.11245 |      0.0516 |     -0.06085 |    min |     -0.00% |
|                Max cumulative merge time across primary shard |              |     0.11245 |      0.0516 |     -0.06085 |    min |     -0.00% |
|                                                    error rate |       search |    0.655664 |           0 |     -0.65566 |      % | -10000.00% |

After (same sample):


|                                                        Metric |         Task |    Baseline |   Contender |         Diff |   Unit |   Diff % |
|--------------------------------------------------------------:|-------------:|------------:|------------:|-------------:|-------:|---------:|
|                    Cumulative indexing time of primary shards |              |   0.0355667 |      0.0667 |      0.03113 |    min |  +87.54% |
|             Max cumulative indexing time across primary shard |              |   0.0355667 |      0.0667 |      0.03113 |    min |  +87.54% |
|                       Cumulative merge time of primary shards |              |     0.11245 |      0.0516 |     -0.06085 |    min |  -54.11% |
|                Max cumulative merge time across primary shard |              |     0.11245 |      0.0516 |     -0.06085 |    min |  -54.11% |
|                                                    error rate |       search |    0.655664 |           0 |     -0.65566 |      % | -100.00% |

Relates #1333

@DJRickyB DJRickyB added bug Something's wrong :Reporting Command line reporting :internal Changes for internal, undocumented features: e.g. experimental, release scripts labels Oct 1, 2021
@DJRickyB DJRickyB added this to the 2.3.0 milestone Oct 1, 2021
@DJRickyB DJRickyB requested a review from b-deam October 1, 2021 01:08
@DJRickyB DJRickyB self-assigned this Oct 1, 2021
@DJRickyB
Copy link
Contributor Author

DJRickyB commented Oct 1, 2021

marked as :internal to keep it out of the release notes, minor bug fix for an as-yet-unreleased feature.

@@ -14,10 +14,12 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
# pylint: disable=protected-access
Copy link
Contributor Author

Choose a reason for hiding this comment

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

saw this elsewhere, adopted here rather than making _diff() public

Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

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

Thanks! Now I'm curious however, why do we use convert.factor(100) for error rates?

@DJRickyB
Copy link
Contributor Author

DJRickyB commented Oct 1, 2021

Now I'm curious however, why do we use convert.factor(100) for error rates?

We express our error rates as a percentage. It might be able to be refactored away from the formatter (so we just use the identity, the default) but I'm leaving that as out-of-scope for this change

@pquentin
Copy link
Member

pquentin commented Oct 1, 2021

Thanks! Yes, definitely out of scope, I wasn't suggesting a refactoring.

@DJRickyB DJRickyB merged commit cca8255 into elastic:master Oct 4, 2021
@DJRickyB DJRickyB deleted the diff-fix branch October 4, 2021 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something's wrong :internal Changes for internal, undocumented features: e.g. experimental, release scripts :Reporting Command line reporting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants