[BEAM-2958] Adding a top-level user agent string to PipelineOptions by youngoli · Pull Request #3915 · apache/beam

youngoli · 2017-09-28T00:24:51Z

Follow this checklist to help us incorporate your contribution quickly and easily:

Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
Each commit in the pull request should have a meaningful subject line and body.
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Run mvn clean verify to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

Adding a string to PipelineOptions that can be used for setting a user agent string with information about the distribution of Beam being used. That string can be sent to external services too.

This PR is probably not completely done yet. While the actual user agent string is in and working, more might have to be done to replace BigTableIO and DataflowRunner usages of user agents to use this new one. I'm still working on that right now.

youngoli · 2017-09-28T00:25:01Z

R: @lukecwik

lukecwik · 2017-09-28T16:38:30Z

retest this please

lukecwik

Minor comments, LGTM after comments and passing Jenkins maven clean install.

Do you plan to make the BQIO/Dataflow Runner changes in separate PRs?

lukecwik · 2017-09-28T16:39:22Z

sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptions.java

+   */
+  @Description("A user agent string describing the pipeline to external services. "
+      + "The default name is \"[name]/[version]\""
+      + " where name and version are properties of the Beam release.")


of the Beam release. -> of the Apache Beam release.

lukecwik · 2017-09-28T16:41:15Z

sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptions.java

+
+  /**
+   * Returns a user agent string constructed from {@link ReleaseInfo#getName()} and
+   * {@link ReleaseInfo#getVersion()}, in the format "[name]/[version]".


"[name]/[version]" -> {@code [name]/[version]}

"[name]/[version]" -> {@code [name]/[version]}

lukecwik · 2017-09-28T16:41:55Z

sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptions.java

+    @Override
+    public String create(PipelineOptions options) {
+      ReleaseInfo info = ReleaseInfo.getReleaseInfo();
+      return


nit: whitespace around return looks awkward

coveralls · 2017-09-28T19:12:11Z

Coverage decreased (-0.0003%) to 69.572% when pulling d716696 on youngoli:bugfix-2958 into da531b7 on apache:master.

For Bigtable I modified it to set the user agent in BigtableOptions from the user agent in PipelineOptions, rather than directly from ReleaseInfo. For DataflowRunner I overwrite the default user agent when setting up the DataflowOptions, using the name and version from DataflowRunnerInfo. Currently that should result in the same user agent as the default because DataflowRunnerInfo just inherits the name and version from ReleaseInfo. But if that name and version ever changes for Dataflow it will reflect in the user agent.

lukecwik · 2017-09-28T20:19:14Z

sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptions.java

+    @Override
+    public String create(PipelineOptions options) {
+      ReleaseInfo info = ReleaseInfo.getReleaseInfo();
+      return String.format("%s/%s", info.getName(), info.getVersion()).replace(" ", "_");


Is there a reason why we replace " " with "_"?

That's how BigtableIO originally had it when reading from ReleaseInfo. Why it did that replacement I'm not sure.

It turns out they are following https://www.ietf.org/rfc/rfc2616.txt where:

user agent = 1*(product | comment) product = token ["/" product-version] product-version = token

Add to the user agent comment that this field should represent a user agent as per RFC2616 and give the BNF form that I describe above. A token is limited to any characters not including a separator.

Accidentally used a @code tag in a string instead of javadoc.

youngoli · 2017-09-29T17:42:52Z

retest this please

lukecwik

Minor comments.

lukecwik · 2017-09-29T22:23:07Z

sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptions.java

+   * product            = token ["/" product-version]
+   * product-version    = token
+   * </code></pre>
+   * Where a token is a series of without a separator.


Awkward sentence: Where a token is a series of without a separator.

Whoops, forgot a word.

lukecwik · 2017-09-29T22:23:19Z

sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptions.java

+   * </code></pre>
+   * Where a token is a series of without a separator.
+   *
+   * <p>The string defaults to {@code [name]/[version]} based on the properties of the Beam release.


Beam -> Apache Beam

lukecwik · 2017-09-29T22:23:49Z

sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptions.java

+
+  /**
+   * Returns a user agent string constructed from {@link ReleaseInfo#getName()} and
+   * {@link ReleaseInfo#getVersion()}, in the format "[name]/[version]".


"[name]/[version]" -> {@code [name]/[version]}

lukecwik · 2017-10-03T18:44:31Z

Failed in known Gearpump test issue. LGTM, merging

youngoli added 2 commits September 27, 2017 12:24

[BEAM-2958] Adding user agent string to PipelineOptions.

a70ff0b

[BEAM-2958] Adding a unit test for UserAgentFactory.

e56ebf6

lukecwik reviewed Sep 28, 2017

View reviewed changes

[BEAM-2958] Fixing nits.

d716696

lukecwik reviewed Sep 28, 2017

View reviewed changes

youngoli added 2 commits September 28, 2017 13:58

[BEAM-2958] Quick fix.

a18907b

Accidentally used a @code tag in a string instead of javadoc.

[BEAM-2958] Referring to RFC2616 in documentation of user agent field.

208a676

lukecwik reviewed Sep 29, 2017

View reviewed changes

youngoli added 2 commits September 29, 2017 16:01

[BEAM-2958] Fixing nits.

8f696e6

[BEAM-2958] Quick fix, forgot to update comment.

befaab2

asfgit closed this in ba5bee6 Oct 3, 2017

Conversation

youngoli commented Sep 28, 2017

Uh oh!

youngoli commented Sep 28, 2017

Uh oh!

lukecwik commented Sep 28, 2017

Uh oh!

lukecwik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukecwik Sep 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Sep 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youngoli commented Sep 29, 2017

Uh oh!

lukecwik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukecwik Sep 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukecwik commented Oct 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lukecwik Sep 29, 2017 •

edited

Loading

lukecwik Sep 29, 2017 •

edited

Loading