Skip to content

Add more variables to template to compute sampler names#5869

Merged
vlsi merged 1 commit intoapache:masterfrom
FSchumacher:add-more-variables-to-template
Jun 5, 2023
Merged

Add more variables to template to compute sampler names#5869
vlsi merged 1 commit intoapache:masterfrom
FSchumacher:add-more-variables-to-template

Conversation

@FSchumacher
Copy link
Contributor

Description

Add more variables to the mini template language to name samplers when recording a test via HTTPS Script recorder

The new variables are

  • method - HTTP method of the request
  • host - host of the URL recorded (called domain in the sampler)
  • scheme - scheme of the URL recorded (called protocol in the sampler)
  • port - port of the URL recorded
  • url - URL as recorded

Motivation and Context

In Issue #5820 a user asked to provide more variables, specifically the url.

How Has This Been Tested?

Added a unit test

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • My code follows the code style of this project.
  • I have updated the documentation accordingly.

@FSchumacher FSchumacher force-pushed the add-more-variables-to-template branch 2 times, most recently from 4bf162b to 942f14d Compare April 29, 2023 15:30
Comment on lines +380 to +388
Object[] valuesArray;
if (!HTTPConstants.CONNECT.equals(request.getMethod()) && isNumberRequests()) {
sampler.setName(MessageFormat.format(format, prefix, sampler.getPath(), incrementRequestNumberAndGet()));
valuesArray = values.toArray(new Object[values.size() + 1]);
valuesArray[values.size()] = incrementRequestNumberAndGet();
} else {
sampler.setName(MessageFormat.format(format, prefix, sampler.getPath()));
valuesArray = values.toArray();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just wonder: is there a real reason to allow "request number" only in case the method is not CONNECT?
What if we always supply the "request number", and we increase "request number" only in case the method matches?

The dance with toArray[size+1] looks strange.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To answer your first request (not really), I have no idea, I guess, it was to filter out proxy requests.
About your second concern. I wanted to get around using a full blown ArrayList, but it is probably better to go for readability and use it anyway.

Comment on lines +394 to +403
.replaceAll("#\\{method([,}])", "{2$1")
.replaceAll("#\\{host([,}])", "{3$1")
.replaceAll("#\\{scheme([,}])", "{4$1")
.replaceAll("#\\{port([,}])", "{5$1")
.replaceAll("#\\{url([,}])", "{6$1")
.replaceAll("#\\{counter([,}])", "{7$1");
Copy link
Collaborator

@vlsi vlsi May 5, 2023

Choose a reason for hiding this comment

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

In theory, users might have used {2} in their scripts for "counter", and if we move counter to {7} that would break their scripts.

I wonder if we can keep #{counter} at position {2} even though that was not properly documented.

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, that would be a nicer option, but I didn't find a way to enable that easily, so I opted to implicitly say: it was never documented and merely an implementation detail.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How about making a first round of replaces that replaces all {[0-9]+} with their escaped representations?
In other words, let's keep {0} literal since that might be something the user wants to have in the sample name.

A yet another option is to escape all { characters to avoid MessageFormat failures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean, if the user sets the format string to #{counter} => {0} and expects JMeter to not touch the {0}?
We had no reports into that direction, so I think the chances are low, it is a problem for many people.

Copy link
Collaborator

Choose a reason for hiding this comment

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

expects JMeter to not touch the {0}?

That is right. I expect if we expose only #{...}, then we should avoid touching things like {0}.

@vlsi
Copy link
Collaborator

vlsi commented May 6, 2023

TL;DR: adding new language constructs to existing languages is not easy. We should refrain from extending existing languages.

MessageFormat.format("Hello {")

yields

java.lang.IllegalArgumentException: Unmatched braces in the pattern.
	at java.base/java.text.MessageFormat.applyPattern(MessageFormat.java:521)
	at java.base/java.text.MessageFormat.<init>(MessageFormat.java:371)
	at java.base/java.text.MessageFormat.format(MessageFormat.java:860)

MessageFormat.format("Doesn't contain quote") yields Doesnt contain quote.

So we should either declare that the format uses MessageFormat patterns (with all the ' for escapes), or we should declare we use a completely different language with its own escapes.

If we go with MessageFormat, then we should assume people could use ' for literal parts like Doesn''t contain quote for expressing Doesn't.

That means we must not blindly replace #{...} as it might be inside quotes.
If we continue with MessageFormat, we should probably use proper quote handling when replacing #{url}-like named placeholders.

@vlsi
Copy link
Collaborator

vlsi commented May 9, 2023

The documentation did mention MessageFormat which implies {0} was somewhat documented.

I would like to move forward here, however, it is sad that we expose unintended placeholders

@vlsi vlsi added this to the 5.6 milestone May 11, 2023
@vlsi
Copy link
Collaborator

vlsi commented May 19, 2023

@FSchumacher , are you goring to merge this? Do you think it is mergeable?

The new variables are
 * method - HTTP method of the request
 * host   - host of the URL recorded (called domain in the sampler)
 * scheme - scheme of the URL recorded (called protocol in the sampler)
 * port   - port of the URL recorded
 * url    - URL as recorded

Issue apache#5820

Co-authored-by: Alex Schwartz <alexschwartz01@gmail.com>

Guard message formats that might have been given by user

As Vladimir pointed out, we might have users, that construct
a format string with a part, that might be interpreted by
MessageFormat. So let us guard those.

asdf
@vlsi vlsi force-pushed the add-more-variables-to-template branch from d61b0ec to ae8c1ec Compare June 5, 2023 14:41
@vlsi vlsi merged commit ae8c1ec into apache:master Jun 5, 2023
@vlsi
Copy link
Collaborator

vlsi commented Jun 5, 2023

What do you mean by contributor label?

@vlsi
Copy link
Collaborator

vlsi commented Jun 5, 2023

I added your name via Co-authored-by in the commit message (see ae8c1ec, https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors), and the link to your profile exists in changes.xml.

WDYT?

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.

2 participants