Support additional validation of keys and values

[jimshowalter]

As discussed in open-telemetry/opentelemetry-java#2935, we need to restrict the keys and values in our baggage, to avoid having it become an untyped free-for-all dumping-ground, and to keep it from exceeding the size limits).

It seems like this would be a nice thing to add to the specification, so all implementations support it.

[arminru]

#504 already covers span/link/event/resource attributes as well as metric labels. I suppose we will want to have the same or similar defaults and mechanism for configuring it also for baggage.
Can you please specify in more detail what kind of validation you'd expect to see? Only size constraints or more than that?

[jimshowalter]

From the code walkthrough for our implementation:

INTRODUCTION

XYZ Corp is adopting OpenTelemetry (https://github.com/open-telemetry/opentelemetry-specification) for context propagation (https://medium.com/jaegertracing/embracing-context-propagation-7100b9b6029a).

OpenTelemetry supports traces (the entire set of calls from the first to the last), and spans (one call in the trace).

OpenTelemetry also helpfully provides a way to attach "baggage" to the context that propagates through OpenTelemetry context mechanisms. (This baggage is not associated directly with any traces or spans, nor is it propagated with them, unless the appropriate propagator is enabled.) The baggage consists of a map of key/value pairs.

Baggage can hold anything (up to defined limits, https://w3c.github.io/baggage/#limits).

Unfortunately, with 100s of services, and without some governance, a ubiquitously available and untyped key/value map will quickly degenerate into a free-for-all dumping ground, and could even exceed the maximum sizes, bringing all festivities to a halt.

This wiki documents how we plan to avoid having that happen at XYZ Corp.

REQUIREMENTS

• Every span within a trace shares the same baggage.
• Services should only set allowed fields in the baggage.
• The fields in the baggage should be set as early as possible, by whichever service is capable of setting them. (This will likely often be the publicly-facing API gateway, but there could be exceptions.)
• A service should not change any fields in the baggage that are already set.
• If a service sees that some fields in the baggage are not set, it should try to fill in those fields with the information it has available.

SCHEMA

We are constraining the key/value pairs in the baggage to conform to a schema.

Schema File

The schema is defined in a text file.

Each line in the schema file defines the schema for one key/value pair.

The keys are simply unique names, for example "uuid" or "route".

The values are constrained by regular expressions.

The regular expressions are fully-specified, meaning they start with a '^' and end with a '$'. (If one or both of those are missing, they are automatically added.)

The keys and regular expressions are separated by a colon.

For example:

number:[0-9]+
uuid:[0-9a-fA-F]{8}[-][0-9a-fA-F]{4}[-][0-9a-fA-F]{4}[-][0-9a-fA-F]{4}[-][0-9a-fA-F]{12}

There is no need to constrain the keys (other than making sure they are unique) because OpenTelemetry makes sure they are not null or empty (or will once open-telemetry/opentelemetry-java#2948 is fixed).

Loading

At startup, the schema file is loaded from a resource on the classpath, parsed, and turned into an XyzBaggageSchema object:

public class XyzBaggageSchema {
 
  private final Map<String, XyzBaggageKeyValueDefinition> definitions;
 
  private static Map<String, XyzBaggageKeyValueDefinition> load(String schemaFile) {
    ...
  }
 
  public XyzBaggageSchema(String schemaFile) {
    definitions = load(schemaFile);
  }

For each line in the schema file, the parser:

1. Trims the line.
2. If the result is empty, continues.
3. If the result is not empty, splits the trimmed line on the first colon.
4. If there aren't two segments after the splitting, throws an exception.
5. Trims the first segment and assigns it to a key variable.
6. Checks if the key variable is already in the definitions and, if so, throws an exception.
7. Constructs an instance of XyzBaggageKeyValueDefinition, passing in the key and the trimmed second segment as the value regular expression.

The XyzBaggageKeyValueDefinition constructor performs some more work:

1. If the trimmed value regular expression is empty, throws an exception.
2. Otherwise:
3. Adds a leading '^' if not already present.
4. Adds a trailing '$' if not already present.
5. Compiles the regular expression.

BUILDER

We are implementing a custom baggage builder, XyzBaggageBuilder, in order to validate that only allowed fields are set:

@Override
public BaggageBuilder put(String key, String value, BaggageEntryMetadata entryMetadata) {
  schema.assertValidKeyAndValue(key, value); <<< THIS IS THE ONLY LINE WE WANTED TO ADD.
  builder.put(key.trim(), value.trim(), entryMetadata);
  return this;
}

Yes, this can be easily defeated with reflection, but by the time we are done with the end-to-end implementation, creating and populating baggage will be handled in the API gateway code, and developers outside that team will never need to see the baggage creation.

Unfortunately, we have to implement our custom builder by composition, because the base BaggageBuilder is a static class that refers to fields we can't access in a non-nested subclass:

public class XyzBaggageBuilder implements BaggageBuilder {
  private final BaggageBuilder builder = ImmutableBaggage.builder();

Ideally we could just register a custom validator with BaggageBuilder and not have to subclass at all, and we filed enhancement requests for that (open-telemetry/opentelemetry-java#2935 and #1490), but there's no guarantee it will ever happen.

Similar to the non-custom BaggageBuilder, our custom baggage builder has static creator methods, but in our case there are two: one for when there is no existing baggage, and one for when there is existing baggage (both of our methods require an XyzBaggageSchema):

public static XyzBaggageBuilder builder(final XyzBaggageSchema schema) {
  return new XyzBaggageBuilder(schema, null);
}
 
public static XyzBaggageBuilder builder(final XyzBaggageSchema schema, final Baggage existingBaggage) {
  return new XyzBaggageBuilder(schema, existingBaggage);
}

(It's safe to call the one with existingBaggage even when that is null, because the constructor returns right away for nulls.)

When the existingBaggage is not null, its contents are copied to the builder held by composition in the XyzBaggageBuilder.

private XyzBaggageBuilder(@NotNull final XyzBaggageSchema schema, @Nullable final Baggage existingBaggage) {
  if (schema == null) {
    throw new XyzOpenTelemetryException("Null schema");
  }
  this.schema = schema;
  if (existingBaggage == null) {
    return;
  }
  // For both forward and backward compatibility, we don't validate keys when copying an existing
  // baggage in order to add/modify/delete. We just copy all of the fields, some of which might be
  // obsolete, and some of which might be ahead of our current set of keys.
  existingBaggage.forEach((key, entry) -> builder.put(key, entry.getValue(), entry.getMetadata()));
}

USAGE

XyzBaggageBuilderTest.testWhileDemonstratingUseWithHeaders demonstrates how we envision our custom builder being used:

void testWhileDemonstratingUseWithHeaders() {
  HttpServletRequest request = mock(HttpServletRequest.class);
  when(request.getHeader("number")).thenReturn("1234");
  when(request.getHeader("uuid")).thenReturn("6a2f41a3-c54c-fce8-32d2-0324e1c32e22");
  when(request.getHeader("unknown-key")).thenReturn("some-value");
 
  //////////////////////////////////////////////////////////////////////////////////////////////
  // This would be somewhere in the API gateway and/or service code, when a request first hits an
  // endpoint. The schema would be read once, on startup, and stored in a static. The name of the
  // schema definition file would of course not be "test-schema.txt".
  String SCHEMA_RESOURCE_NAME = "test-schema.txt";
  XyzBaggageSchema SCHEMA_READ_INTO_A_STATIC_DURING_STARTUP = new XyzBaggageSchema(SCHEMA_RESOURCE_NAME);
  // Normally we wouldn't do this, but we want to show that existing values are left alone.
  Baggage existingBaggage = Baggage.builder().put("number", "5678").build();
  // Normally we would pass Baggage.current() into the builder constructor instead of
  // existingBaggage, but we want to show that existing values are left alone.
  XyzBaggageBuilder builder = XyzBaggageBuilder.builder(SCHEMA_READ_INTO_A_STATIC_DURING_STARTUP, existingBaggage);
  for (XyzBaggageKeyValueDefinition definition : builder.schema().definitions().values()) {
    String value = request.getHeader(definition.key());
    if (value != null) {
      if (existingBaggage.getEntryValue(definition.key()) == null) {
        builder.put(definition.key(), value);
      }
    }
  }
  Context.current().with(builder.build()).makeCurrent();
  // End of API gateway and/or service code.
  //////////////////////////////////////////////////////////////////////////////////////////////
 
  assertEquals("5678", Baggage.fromContext(Context.current()).getEntryValue("number"));
  assertEquals(
      "6a2f41a3-c54c-fce8-32d2-0324e1c32e22",
      Baggage.fromContext(Context.current()).getEntryValue("uuid"));
}

MISCELLANEOUS CODE

There are only two other classes in the implementation:

• XyzOpenTelemetryException: This is just a vanilla subclass of RuntimeException. It's a placeholder until the code is merged into gateway code and services code, at which point a better exception will be used.
• XyzBaggageStringUtils: These are copied from io.opentelemetry.api.baggage.ImmutableBaggage.isKeyValid and io.opentelemetry.api.internal.StringUtils. This is necessary because isKeyValid is a private static in ImmutableBaggage, and StringUtils is from an internal API. We filed an enhancement request to move the statics to StringUtils and move StringUtils to a public package (Move validation methods to public api opentelemetry-java#2944).

[jkwatson]

Just one clarification/misconception I saw in your writeup:

OpenTelemetry also helpfully carries "baggage" along in the traces and spans, where the baggage consists of a map of key/value pairs.

This is not true. What OpenTelemetry does is to provide a way to attach Baggage to the context which propagates through the OpenTelemetry context mechanisms. This baggage is not associated directly with any spans or traces, nor is it propagated with them, unless the appropriate propagator is enabled.

[jimshowalter]

If you look at all of the above from the standpoint of developer friendliness, really all we need are two things:

• Ability to define allowed keys and specify regular expressions for values, and have those definitions be enforced by the builder.
• Ability to add allowed keys and valid values to the baggage if they aren't already present, and make the updated baggage be the current baggage.

[jimshowalter]

Thank you for the clarification! I updated our walkthrough to be more precise.

[jimshowalter]

For long-running services, we might add an invalidation mechanism, so the service rereads the schema file and updates the in-memory schema.

[jimshowalter]

In some cases regular expressions might not be powerful/flexible enough, so it would be nice if a validator could be specified (or discovered).

[jimshowalter]

We also considered imposing the constraints in our custom publisher, but it feels wrong to wait to the publish step before throwing an exception. We want to flag it at the point of (mis)use.