Skip to content

Conjure User-Agents supports arbitrary comment data #1286

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

Merged
merged 5 commits into from
Feb 4, 2025
Merged

Conjure User-Agents supports arbitrary comment data #1286

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f71923c
Conjure User-Agents supports arbitrary comment data
Jan 30, 2025
06e730a
Move comment to Agent
Jan 31, 2025
c22647f
experiment with allowed commas
Jan 31, 2025
bf52874
Add generated changelog entries
svc-changelog Feb 3, 2025
688a72b
annotations and javadoc
Feb 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions changelog/@unreleased/pr-1286.v2.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
type: improvement
improvement:
description: Conjure User-Agents supports arbitrary comment metadata, for observability
metadata similar to the existing `nodeId` parameter.
links:
- https://github.com/palantir/conjure-java-runtime-api/pull/1286
65 changes: 50 additions & 15 deletions service-config/src/main/java/com/palantir/conjure/java/api/config/service/UserAgent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,13 @@

package com.palantir.conjure.java.api.config.service;

import com.palantir.logsafe.Preconditions;
import com.google.common.collect.ImmutableList;
import com.palantir.logsafe.Safe;
import com.palantir.logsafe.SafeArg;
import com.palantir.logsafe.exceptions.SafeIllegalArgumentException;
import java.util.List;
import java.util.Optional;
import java.util.stream.Stream;
import org.immutables.value.Value;

/**
Expand All @@ -33,7 +35,18 @@
public interface UserAgent {

/** Identifies the node (e.g., IP address, container identifier, etc) on which this user agent was constructed. */
Optional<String> nodeId();
@Value.Lazy
default Optional<@Safe String> nodeId() {
if (primary().comments().isEmpty()) {
// fast path to avoid stream overhead
return Optional.empty();
}
return primary().comments().stream()
.filter(item -> item.startsWith("nodeId:"))
.map(item -> item.substring(7).trim())
.filter(UserAgents::isValidNodeId)
.findFirst();
}

/** The primary user agent, typically the name/version of the service initiating an RPC call. */
Agent primary();
Expand All @@ -45,8 +58,15 @@ public interface UserAgent {
List<Agent> informational();

/** Creates a new {@link UserAgent} with the given {@link #primary} agent and originating node id. */
static UserAgent of(Agent agent, String nodeId) {
return ImmutableUserAgent.builder().nodeId(nodeId).primary(agent).build();
static UserAgent of(Agent agent, @Safe String nodeId) {
UserAgents.checkNodeId(nodeId);
List<String> comments = Stream.concat(
agent.comments().stream().filter(item -> !item.startsWith("nodeId:")),
Stream.of("nodeId:" + nodeId))
.toList();
return ImmutableUserAgent.builder()
.primary(Agent.of(agent.name(), agent.version(), comments))
.build();
}

/**
Expand Down Expand Up @@ -75,26 +95,28 @@ default UserAgent addAgent(Agent agent) {
return ImmutableUserAgent.builder().from(this).addInformational(agent).build();
}

@Value.Check
default void check() {
if (nodeId().isPresent()) {
Preconditions.checkArgument(
UserAgents.isValidNodeId(nodeId().get()),
"Illegal node id format",
SafeArg.of("nodeId", nodeId().get()));
}
}

/** Specifies an agent that participates (client-side) in an RPC call in terms of its name and version. */
@Value.Immutable
@ImmutablesStyle
@Safe
interface Agent {
String DEFAULT_VERSION = "0.0.0";

@Safe
String name();

@Safe
String version();

/**
* <a href="https://datatracker.ietf.org/doc/html/rfc7231#section-5.5.3>rfc7231 section-5.5.3</a> comment
* metadata (as described by
* <a href="https://datatracker.ietf.org/doc/html/rfc7230#section-3.2.6">rfc7230 section-3.2.6</a>)
* for additional diagnostic information. Note that this library provides a much stricter set of allowed
* characters within comments than the linked RFCs to reduce complexity.
*/
List<@Safe String> comments();
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to validate comments in the check() method as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, there are two factory methods, only one takes comments, and it validates them before passing args. This way we avoid unnecessary work in the common case.

Copy link
Contributor

@bjlaub bjlaub Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

sure, but the same could be said about name and version, no? should we just remove the check() method? i think this is more of a question of if anyone does/will use ImmutableAgent.builder() directly.

This way we avoid unnecessary work in the common case.

what's the "common case" here? using the factory that does not accept comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think this is more of a question of if anyone does/will use ImmutableAgent.builder() directly.

Right, ImmutableAgent is package private, so it can't be used from outside of this module (assuming no package-clobbering hackery)

what's the "common case" here? using the factory that does not accept comments?

Correct, the existing factory method that folks already use today, which does not accept comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

avoid unnecessary work

isn't this just a conditional on comments().size()?

Copy link
Contributor

Choose a reason for hiding this comment

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

ImmutableAgent is package private

ah okay, i missed that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep!

In general I'm a bit biased against immutables @Check methods as well, since they add a public method to the interfaces API which isn't meant to be called by consumers of the interface.


@Value.Check
default void check() {
if (!UserAgents.isValidName(name())) {
Expand All @@ -107,10 +129,23 @@ default void check() {
}
}

static Agent of(String name, String version) {
static Agent of(@Safe String name, @Safe String version) {
return ImmutableAgent.builder()
.name(name)
.version(UserAgents.isValidVersion(version) ? version : DEFAULT_VERSION)
.build();
}

static Agent of(@Safe String name, @Safe String version, @Safe Iterable<@Safe String> comments) {
ImmutableList<String> immutableComments = ImmutableList.copyOf(comments);
for (int i = 0; i < immutableComments.size(); i++) {
String comment = immutableComments.get(i);
UserAgents.checkComment(comment);
}
return ImmutableAgent.builder()
.name(name)
.version(UserAgents.isValidVersion(version) ? version : DEFAULT_VERSION)
.comments(immutableComments)
.build();
}
}
Expand Down
106 changes: 76 additions & 30 deletions service-config/src/main/java/com/palantir/conjure/java/api/config/service/UserAgents.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,14 @@

import com.google.common.base.CharMatcher;
import com.google.common.base.Splitter;
import com.google.common.collect.ImmutableList;
import com.palantir.logsafe.Preconditions;
import com.palantir.logsafe.Safe;
import com.palantir.logsafe.SafeArg;
import com.palantir.logsafe.exceptions.SafeIllegalArgumentException;
import com.palantir.logsafe.logger.SafeLogger;
import com.palantir.logsafe.logger.SafeLoggerFactory;
import java.util.HashMap;
import java.util.Map;
import java.util.List;
import java.util.Optional;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
Expand All @@ -45,18 +46,21 @@ public final class UserAgents {
Pattern.compile("^[0-9]+(?:\\.[0-9]+)*(?:-rc[0-9]+)?(?:-[0-9]+-g[a-f0-9]+)?$");
private static final Pattern SEGMENT_PATTERN =
Pattern.compile(String.format("(%s)/(%s)( \\((.+?)\\))?", NAME_REGEX, LENIENT_VERSION_REGEX));
private static final Splitter COMMA_OR_SEMICOLON_SPLITTER =
Splitter.on(CharMatcher.anyOf(",;").precomputed()).trimResults().omitEmptyStrings();
private static final Splitter SEMICOLON_SPLITTER =
Splitter.on(CharMatcher.is(';').precomputed()).trimResults().omitEmptyStrings();
Comment on lines +49 to +50
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the most recent commit, I've updated this to no longer split on commas. I haven't found examples where the comma is used as a delimiter like semicolon, but it is used within the common string KHTML, like Gecko.
Everywhere that we encoded nodeId was a single component, and that wouldn't be impacted (though I don't think we actually use the nodeId component anywhere these days): (nodeId:value)

private static final CharMatcher COMMENT_VALID_CHARS = CharMatcher.inRange('a', 'z')
.or(CharMatcher.inRange('A', 'Z'))
.or(CharMatcher.inRange('0', '9'))
.or(CharMatcher.anyOf(".-:_/ ,"))
.precomputed();

private UserAgents() {}

/** Returns the canonical string format for the given {@link UserAgent}. */
public static String format(UserAgent userAgent) {
@Safe
public static String format(@Safe UserAgent userAgent) {
StringBuilder formatted = new StringBuilder(64); // preallocate larger buffer for longer agents
formatSimpleAgent(userAgent.primary(), formatted);
if (userAgent.nodeId().isPresent()) {
formatted.append(" (nodeId:").append(userAgent.nodeId().get()).append(')');
}
for (UserAgent.Agent informationalAgent : userAgent.informational()) {
formatted.append(' ');
formatSimpleAgent(informationalAgent, formatted);
Expand All @@ -68,6 +72,17 @@ private static void formatSimpleAgent(UserAgent.Agent agent, StringBuilder outpu
output.ensureCapacity(
output.length() + 1 + agent.name().length() + agent.version().length());
output.append(agent.name()).append('/').append(agent.version());
List<String> comments = agent.comments();
if (!comments.isEmpty()) {
output.append(" (");
for (int i = 0; i < comments.size(); i++) {
if (i > 0) {
output.append("; ");
}
output.append(comments.get(i));
}
output.append(')');
}
}

/**
Expand All @@ -76,7 +91,7 @@ private static void formatSimpleAgent(UserAgent.Agent agent, StringBuilder outpu
*
* <p>Valid user agent strings loosely follow RFC 7230 (https://tools.ietf.org/html/rfc7230#section-3.2.6).
*/
public static UserAgent parse(String userAgent) {
public static UserAgent parse(@Safe String userAgent) {
Preconditions.checkNotNull(userAgent, "userAgent must not be null");
return parseInternal(userAgent, false /* strict */);
}
Expand All @@ -85,7 +100,7 @@ public static UserAgent parse(String userAgent) {
* Like {@link #parse}, but never fails and returns the primary agent {@code unknown/0.0.0} if no valid primary
* agent can be parsed.
*/
public static UserAgent tryParse(String userAgent) {
public static UserAgent tryParse(@Safe String userAgent) {

return parseInternal(userAgent == null ? "" : userAgent, true /* lenient */);
}
Expand All @@ -97,7 +112,7 @@ public static UserAgent tryParse(String userAgent) {
* This is functionally similar to calling `tryParse(userAgent).primary().name()`, but optimized to not use
* regular expressions.
*/
public static String tryParsePrimaryName(String userAgent) {
public static String tryParsePrimaryName(@Safe String userAgent) {
if (userAgent == null) {
return "unknown";
}
Expand Down Expand Up @@ -132,7 +147,7 @@ public static String tryParsePrimaryName(String userAgent) {
return userAgent.substring(lindex, split);
}

private static UserAgent parseInternal(String userAgent, boolean lenient) {
private static UserAgent parseInternal(@Safe String userAgent, boolean lenient) {
ImmutableUserAgent.Builder builder = ImmutableUserAgent.builder();

Matcher matcher = SEGMENT_PATTERN.matcher(userAgent);
Expand All @@ -142,18 +157,14 @@ private static UserAgent parseInternal(String userAgent, boolean lenient) {
String version = matcher.group(2);
Optional<String> comments = Optional.ofNullable(matcher.group(4));

UserAgent.Agent agent = UserAgent.Agent.of(
name, version, comments.map(UserAgents::parseComments).orElseGet(ImmutableList::of));
if (!foundFirst) {
// primary
builder.primary(UserAgent.Agent.of(name, version));
comments.ifPresent(c -> {
Map<String, String> parsedComments = parseComments(c);
if (parsedComments.containsKey("nodeId")) {
builder.nodeId(parsedComments.get("nodeId"));
}
});
builder.primary(agent);
} else {
// informational
builder.addInformational(UserAgent.Agent.of(name, version));
builder.addInformational(agent);
}

foundFirst = true;
Expand All @@ -177,17 +188,46 @@ private static UserAgent parseInternal(String userAgent, boolean lenient) {
return builder.build();
}

private static Map<String, String> parseComments(String commentsString) {
Map<String, String> comments = new HashMap<>();
for (String comment : COMMA_OR_SEMICOLON_SPLITTER.split(commentsString)) {
String[] fields = comment.split(":");
if (fields.length == 2) {
comments.put(fields[0], fields[1]);
} else {
comments.put(comment, comment);
private static List<String> parseComments(String commentsString) {
List<String> results = SEMICOLON_SPLITTER.splitToList(commentsString);
for (int i = 0; i < results.size(); ++i) {
// In most cases, all comments will be valid, so we avoid stream overhead.
if (!isValidComment(results.get(i))) {
return results.stream().filter(UserAgents::isValidComment).toList();
}
}
return comments;
return results;
}

static void checkComment(@Safe String comment) {
if (comment == null) {
throw new SafeIllegalArgumentException("Comment must not be null");
}
if (comment.isEmpty()) {
throw new SafeIllegalArgumentException("Comment must not be empty");
}
if (comment.startsWith(" ")) {
throw new SafeIllegalArgumentException(
"Comment must not start with whitespace", SafeArg.of("comment", comment));
}
if (comment.endsWith(" ")) {
throw new SafeIllegalArgumentException(
"Comment must not end with whitespace", SafeArg.of("comment", comment));
}
if (!COMMENT_VALID_CHARS.matchesAllOf(comment)) {
throw new SafeIllegalArgumentException(
"Comment contains disallowed characters",
SafeArg.of("allowed", "a-zA-Z0-9.-:_/ "),
SafeArg.of("comment", comment));
}
}

static boolean isValidComment(@Safe String comment) {
return comment != null
&& !comment.isEmpty()
&& !comment.startsWith(" ")
&& !comment.endsWith(" ")
&& COMMENT_VALID_CHARS.matchesAllOf(comment);
}

static boolean isValidName(String name) {
Expand Down Expand Up @@ -227,7 +267,13 @@ static boolean isValidNodeId(String instanceId) {
return NODE_REGEX.matcher(instanceId).matches();
}

static boolean isValidVersion(String version) {
static void checkNodeId(@Safe String instanceId) {
if (!isValidNodeId(instanceId)) {
throw new SafeIllegalArgumentException("Illegal node id format", SafeArg.of("nodeId", instanceId));
}
}

static boolean isValidVersion(@Safe String version) {
if (VersionParser.countNumericDotGroups(version) >= 2 // fast path for numeric & dot only version numbers
|| versionMatchesRegex(version)) {
return true;
Expand Down
Loading