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.

Sign up for GitHub

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

Jump to bottom

add --user-tags as alias for --user-tag #1599

Merged
merged 5 commits into from
Nov 2, 2022
Merged

add --user-tags as alias for --user-tag #1599

merged 5 commits into from
Nov 2, 2022

Conversation

pakio
Copy link
Contributor

@pakio pakio commented Oct 22, 2022

This PR closes #1577, adding a track option --user-tags as an alias for --user-tag.

==== checklist ====

@pakio pakio force-pushed the ISSUE-1577_add-user-tag-alias branch 2 times, most recently from 447e7e0 to 277c303 Compare October 22, 2022 16:27
@pakio pakio force-pushed the ISSUE-1577_add-user-tag-alias branch from 277c303 to 5887733 Compare October 23, 2022 00:44
pakio
pakio commented Oct 23, 2022
View reviewed changes
esrally/rally.py
Comment on lines 653 to +654
"--user-tag",
"--user-tags",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

didn't change the order to keep backward compatibility, in order to be able to set tag by env var USER_TAG.

dliappis
dliappis requested changes Oct 24, 2022
View reviewed changes
Copy link
Contributor

@dliappis dliappis left a comment

Choose a reason for hiding this comment

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

Thank you for raising this!

I left some comments. One thing that is missing IMHO is that we should emit a "Deprecation warning" when --user-tag is use in the CLI. That way people who don't happen to check the migration guide or read docs will be reminded that they should be switching away.

docs/command_line_reference.rst Outdated



When you run ``esrally list races``, this will show up again::
Wesrally list raceshen you run ``esrally list races``, this will show up again::
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this was introduced by accident?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unintended, thank you for the catch 🙏

docs/metrics.rst Outdated
@@ -108,7 +108,7 @@ Rally captures also some meta information for each metric record:
* Node name: If Rally provisions the cluster, it will choose a unique name for each node.
* Source revision: We always record the git hash of the version of Elasticsearch that is benchmarked. This is even done if you benchmark an official binary release.
* Distribution version: We always record the distribution version of Elasticsearch that is benchmarked. This is even done if you benchmark a source release.
* Custom tag: You can define one custom tag with the command line flag ``--user-tag``. The tag is prefixed by ``tag_`` in order to avoid accidental clashes with Rally internal tags.
* Custom tags: You can define one custom tags with the command line flag ``--user-tags``. The tags are prefixed by ``tag_`` in order to avoid accidental clashes with Rally internal tags.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Custom tags: You can define one custom tags with the command line flag ``--user-tags``. The tags are prefixed by ``tag_`` in order to avoid accidental clashes with Rally internal tags.
* Custom tags: You can define custom tags with the command line flag ``--user-tags``. The tags are prefixed by ``tag_`` in order to avoid accidental clashes with Rally internal tags.

docs/migrate.rst Outdated
@@ -446,7 +446,7 @@ If you need lap functionality, the following shell script can be used instead::

for lap in $(seq 1 ${RALLY_LAPS})
do
esrally --pipeline=benchmark-only --user-tag lap:$lap
esrally --pipeline=benchmark-only --user-tags lap:$lap
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be very nice if we could document at the top of this file that --user-tag is now deprecated in favor of --user-tags. The next version can be found in https://github.com/elastic/rally/blob/master/esrally/_version.py#L1 so it'll be effective from 2.6.1.

Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned in #1599 (comment) maybe it's just enough to mention that starting with 2.6.1 --user-tags has been introduced as a "synonym" to --user-tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, sorry I didn't notice this was a migration guide.
I think that might be better to be in the migration guide section for v2.6.1, so I simply reverts this change.

@dliappis dliappis added :Config Config file format changes, new properties, ... tech debt labels Oct 24, 2022
@dliappis
Copy link
Contributor

Thank you for raising this!

I left some comments. One thing that is missing IMHO is that we should emit a "Deprecation warning" when --user-tag is use in the CLI. That way people who don't happen to check the migration guide or read docs will be reminded that they should be switching away.

I discussed with @pquentin and instead of doing what I proposed here, we could just keep the functionality for both i.e. no need for a depreciation warning and allow both for the foreseeable future. Given the large usage of Rally this would probably be the least disruptive change for users.

@pakio
Copy link
Contributor Author

pakio commented Oct 24, 2022

Thank you @dliappis for your discussion and review! Given the result, do you think the current way of migration, adding --user-tags as a secondary parameter looks good to you? I also considered to add as a separate argument so that we can support both USER_TAG and USER_TAGS as well, and which might be more easier to show deprecation message in the future.

@pakio pakio requested review from dliappis and removed request for pquentin October 24, 2022 12:46
@dliappis
Copy link
Contributor

@elasticmachine test this please

dliappis
dliappis approved these changes Oct 31, 2022
View reviewed changes
Copy link
Contributor

@dliappis dliappis left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, this looks great thank you. I've triggered now CI tests, if they are green, I'll merge it in.

@pquentin
Copy link
Member

pquentin commented Nov 2, 2022

@elasticmachine test this please

@pquentin pquentin merged commit 722ef67 into elastic:master Nov 2, 2022
@pquentin pquentin added this to the 2.7.0 milestone Nov 2, 2022
@pquentin pquentin added the enhancement Improves the status quo label Nov 2, 2022
@pakio pakio deleted the ISSUE-1577_add-user-tag-alias branch November 2, 2022 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dliappis dliappis dliappis approved these changes

@pquentin pquentin Awaiting requested review from pquentin

Assignees
No one assigned
Labels
:Config Config file format changes, new properties, ... enhancement Improves the status quo tech debt
Projects
None yet
Milestone
2.7.0
Development

Successfully merging this pull request may close these issues.

Alias --user-tags to --user-tag
3 participants
@pakio @dliappis @pquentin