-
Notifications
You must be signed in to change notification settings - Fork 10
add attribute projections #18
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.
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
Changes from all commits
240246a
37cd762
871f5f8
1607a29
5bb693e
0dd8c99
83e5cfa
f420c93
6c4270d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| plugins { | ||
| `java-library` | ||
| jacoco | ||
| id("org.hypertrace.jacoco-report-plugin") | ||
| id("org.hypertrace.publish-plugin") | ||
| } | ||
|
|
||
| dependencies { | ||
| api("com.google.code.findbugs:jsr305:3.0.2") | ||
| implementation("com.github.f4b6a3:uuid-creator:2.7.7") | ||
|
|
||
| testImplementation("org.junit.jupiter:junit-jupiter:5.6.2") | ||
| } | ||
|
|
||
| tasks.test { | ||
| useJUnitPlatform() | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| package org.hypertrace.core.attribute.service.projection.functions; | ||
|
|
||
| import static java.util.Objects.isNull; | ||
| import static java.util.Objects.requireNonNullElse; | ||
|
|
||
| import javax.annotation.Nullable; | ||
|
|
||
| public class Concatenate { | ||
| private static final String DEFAULT_STRING = ""; | ||
|
|
||
| @Nullable | ||
| public static String concatenate(@Nullable String first, @Nullable String second) { | ||
| if (isNull(first) && isNull(second)) { | ||
| return null; | ||
| } | ||
| return requireNonNullElse(first, DEFAULT_STRING) + requireNonNullElse(second, DEFAULT_STRING); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| package org.hypertrace.core.attribute.service.projection.functions; | ||
|
|
||
| import static java.util.Objects.nonNull; | ||
|
|
||
| import javax.annotation.Nullable; | ||
|
|
||
| public class DefaultValue { | ||
|
|
||
| @Nullable | ||
| public static String defaultString(@Nullable String value, @Nullable String defaultValue) { | ||
| return nonNull(value) && !value.isEmpty() ? value : defaultValue; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| package org.hypertrace.core.attribute.service.projection.functions; | ||
|
|
||
| import static java.util.Objects.nonNull; | ||
|
|
||
| import com.github.f4b6a3.uuid.UuidCreator; | ||
| import com.github.f4b6a3.uuid.creator.rfc4122.NameBasedSha1UuidCreator; | ||
| import java.util.UUID; | ||
| import javax.annotation.Nullable; | ||
|
|
||
| public class Hash { | ||
|
|
||
| /** | ||
| * Unique, randomly generated namespace that should never be used directly. Changing this value | ||
| * would change any existing projections containing a hash and orphan any data persisted against | ||
| * that such as stored entities. | ||
| */ | ||
| private static final NameBasedSha1UuidCreator HASHER = | ||
| UuidCreator.getNameBasedSha1Creator() | ||
| .withNamespace(UUID.fromString("5088c92d-5e9c-43f4-a35b-2589474d5642")); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this one of those magic guids that should never change?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep - I planned on making this unique per tenant, but in the OLAP context we won't have that kind of information (unless provided explicitly). A UUIDv5 requires a namespace (actually, this lib seems to allow without but I don't know what the implications of that are). So this is just a randomly generated UUID - I didn't keep it in its own constant since by design it should never be referenced elsewhere. Thoughts? Clearer as an explicit constant?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually as hardcoded it's better and all consumers can call this one method. Maybe some comments around it?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure, will add a comment
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any relation between this and the UUID generated in entity-service where we had to do the V3/V5 override?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch - I completely forgot that I shared the same namespace with that UUID generator. So right now, there's no relationship, but I shared the namespace because the plan is to eventually remove that generator and have things like entity IDs purely exist as projected attributes. The exact migration there is TBD.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this mean we don't need to change entity-service id generation logic to be not fully dependent on identifying properties?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confused by the double negative here, but the idea is that we're flipping the burden of ID generation. Right now, as I understand it, we upsert an entity by setting attribute values, on an entity object, sending it to entity service, and then reading back the ID from the response. Instead, the plan is to make this formation of an entity config driven rather than code driven - that is, rather than creating the entity explicitly by picking the identifying attributes, we'll have an attribute that is defined as the id attribute for an entity and can be read through the trace client. Entity formation/liveness can then be completely decoupled from enrichment, but use the same mechanism to get the ID. There's a number of TBDs here of course, but that's the general idea. |
||
|
|
||
| @Nullable | ||
| public static String hash(@Nullable String value) { | ||
| return nonNull(value) ? HASHER.create(value).toString() : null; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| package org.hypertrace.core.attribute.service.projection.functions; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertNull; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| class ConcatenateTest { | ||
|
|
||
| @Test | ||
| void concatenatesNormalStrings() { | ||
| assertEquals("foobar", Concatenate.concatenate("foo", "bar")); | ||
| assertEquals("foo", Concatenate.concatenate("foo", "")); | ||
| assertEquals("bar", Concatenate.concatenate("", "bar")); | ||
| } | ||
|
|
||
| @Test | ||
| void concatenatesNullStrings() { | ||
| assertEquals("foo", Concatenate.concatenate("foo", null)); | ||
| assertEquals("bar", Concatenate.concatenate(null, "bar")); | ||
| assertNull(Concatenate.concatenate(null, null)); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| package org.hypertrace.core.attribute.service.projection.functions; | ||
|
|
||
| import static org.hypertrace.core.attribute.service.projection.functions.DefaultValue.defaultString; | ||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertNull; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| public class DefaultValueTest { | ||
| @Test | ||
| void givesDefaultIfStringValueNullOrEmpty() { | ||
| assertEquals("default", defaultString(null, "default")); | ||
| assertEquals("default", defaultString("", "default")); | ||
| assertEquals("foo", defaultString("foo", "default")); | ||
| } | ||
|
|
||
| @Test | ||
| void returnsNullIfNullDefaultGiven() { | ||
| assertNull(defaultString(null, null)); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| package org.hypertrace.core.attribute.service.projection.functions; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertNotEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertNull; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| class HashTest { | ||
|
|
||
| @Test | ||
| void createsMatchingHashesForMatchingInputs() { | ||
| assertEquals(Hash.hash("foo"), Hash.hash("foo")); | ||
| assertNotEquals(Hash.hash("foo"), Hash.hash("bar")); | ||
| } | ||
|
|
||
| @Test | ||
| void hashesNullToNull() { | ||
| assertNull(Hash.hash(null)); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| plugins { | ||
| `java-library` | ||
| jacoco | ||
| id("org.hypertrace.jacoco-report-plugin") | ||
| id("org.hypertrace.publish-plugin") | ||
| } | ||
|
|
||
| dependencies { | ||
| api(project(":attribute-service-api")) | ||
| implementation(project(":attribute-projection-functions")) | ||
| implementation("com.google.guava:guava:29.0-jre") | ||
| testImplementation("org.junit.jupiter:junit-jupiter:5.6.2") | ||
| } | ||
|
|
||
| tasks.test { | ||
| useJUnitPlatform() | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| package org.hypertrace.core.attribute.service.projection; | ||
|
|
||
| import com.google.common.base.Preconditions; | ||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
| import org.hypertrace.core.attribute.service.v1.AttributeKind; | ||
| import org.hypertrace.core.attribute.service.v1.LiteralValue; | ||
|
|
||
| abstract class AbstractAttributeProjection<R> implements AttributeProjection { | ||
|
|
||
| private final AttributeKind resultKind; | ||
| private final List<AttributeKind> argumentKinds; | ||
|
|
||
| protected AbstractAttributeProjection( | ||
| AttributeKind resultKind, List<AttributeKind> argumentKinds) { | ||
| this.resultKind = resultKind; | ||
| this.argumentKinds = argumentKinds; | ||
| } | ||
|
|
||
| @Override | ||
| public LiteralValue project(List<LiteralValue> arguments) { | ||
| Preconditions.checkArgument(arguments.size() == argumentKinds.size()); | ||
| List<Object> unwrappedArguments = new ArrayList<>(argumentKinds.size()); | ||
| for (int index = 0; index < arguments.size(); index++) { | ||
| int argumentIndex = index; | ||
| LiteralValue argumentLiteral = arguments.get(argumentIndex); | ||
| AttributeKind attributeKind = this.argumentKinds.get(argumentIndex); | ||
| Object unwrappedArgument = | ||
| ValueCoercer.fromLiteral(argumentLiteral, attributeKind) | ||
| .orElseThrow( | ||
| () -> | ||
| new IllegalArgumentException( | ||
| String.format( | ||
| "Projection argument %s at index %d could not be converted to expected type %s", | ||
| argumentLiteral, argumentIndex, attributeKind))); | ||
|
|
||
| unwrappedArguments.add(argumentIndex, unwrappedArgument); | ||
| } | ||
| Object unwrappedResult = this.doUnwrappedProjection(unwrappedArguments); | ||
| return ValueCoercer.toLiteral(unwrappedResult, this.resultKind) | ||
| .orElseThrow( | ||
| () -> | ||
| new UnsupportedOperationException( | ||
| String.format( | ||
| "Projection result %s could not be converted to expected type %s", | ||
| unwrappedResult, this.resultKind))); | ||
| } | ||
|
|
||
| protected abstract R doUnwrappedProjection(List<Object> arguments); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| package org.hypertrace.core.attribute.service.projection; | ||
|
|
||
| import java.util.List; | ||
| import org.hypertrace.core.attribute.service.v1.LiteralValue; | ||
|
|
||
| public interface AttributeProjection { | ||
| /** | ||
| * Performs the projection operation with the provided arguments. | ||
| * | ||
| * @param arguments to the projection | ||
| * @return the result of the projection | ||
| * @throws IllegalArgumentException if the provided arguments do not match the expected arity of | ||
| * the projection, can not be converted to the expected input types or produce a result that | ||
| * can't be converted to the expected output type. | ||
| */ | ||
| LiteralValue project(List<LiteralValue> arguments); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| package org.hypertrace.core.attribute.service.projection; | ||
|
|
||
| import static org.hypertrace.core.attribute.service.v1.ProjectionOperator.PROJECTION_OPERATOR_CONCAT; | ||
| import static org.hypertrace.core.attribute.service.v1.ProjectionOperator.PROJECTION_OPERATOR_HASH; | ||
|
|
||
| import java.util.Map; | ||
| import java.util.Optional; | ||
| import org.hypertrace.core.attribute.service.projection.functions.Concatenate; | ||
| import org.hypertrace.core.attribute.service.projection.functions.Hash; | ||
| import org.hypertrace.core.attribute.service.v1.AttributeKind; | ||
| import org.hypertrace.core.attribute.service.v1.ProjectionOperator; | ||
|
|
||
| public class AttributeProjectionRegistry { | ||
|
|
||
| private static final Map<ProjectionOperator, AttributeProjection> PROJECTION_MAP = | ||
| Map.of( | ||
| PROJECTION_OPERATOR_CONCAT, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we need to be able to chain the projections also right? For example, HASH(CONCAT(a, b)) so that we get the id automatically. That can come in later but it's better to keep them in mind.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Talked offline, but summary: yes we need and do support that, it's the responsibility of the layer above to resolve the arguments to literal values, which may include recursive projections. |
||
| new BinaryAttributeProjection<>( | ||
| AttributeKind.TYPE_STRING, | ||
| AttributeKind.TYPE_STRING, | ||
| AttributeKind.TYPE_STRING, | ||
| Concatenate::concatenate), | ||
| PROJECTION_OPERATOR_HASH, | ||
| new UnaryAttributeProjection<>( | ||
| AttributeKind.TYPE_STRING, AttributeKind.TYPE_STRING, Hash::hash)); | ||
|
|
||
| public Optional<AttributeProjection> getProjection(ProjectionOperator projectionOperator) { | ||
| return Optional.ofNullable(PROJECTION_MAP.get(projectionOperator)); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| package org.hypertrace.core.attribute.service.projection; | ||
|
|
||
| import java.util.List; | ||
| import java.util.function.BiFunction; | ||
| import org.hypertrace.core.attribute.service.v1.AttributeKind; | ||
|
|
||
| class BinaryAttributeProjection<T, U, R> extends AbstractAttributeProjection<R> { | ||
| private final BiFunction<T, U, R> projectionImplementation; | ||
|
|
||
| BinaryAttributeProjection( | ||
| AttributeKind resultKind, | ||
| AttributeKind firstArgumentKind, | ||
| AttributeKind secondArgumentKind, | ||
| BiFunction<T, U, R> projectionImplementation) { | ||
| super(resultKind, List.of(firstArgumentKind, secondArgumentKind)); | ||
| this.projectionImplementation = projectionImplementation; | ||
| } | ||
|
|
||
| @Override | ||
| @SuppressWarnings("unchecked") | ||
| protected R doUnwrappedProjection(List<Object> arguments) { | ||
| return this.projectionImplementation.apply((T) arguments.get(0), (U) arguments.get(1)); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| package org.hypertrace.core.attribute.service.projection; | ||
|
|
||
| import java.util.List; | ||
| import java.util.function.Function; | ||
| import org.hypertrace.core.attribute.service.v1.AttributeKind; | ||
|
|
||
| class UnaryAttributeProjection<T, R> extends AbstractAttributeProjection<R> { | ||
| private final Function<T, R> projectionImplementation; | ||
|
|
||
| UnaryAttributeProjection( | ||
| AttributeKind resultKind, | ||
| AttributeKind argumentKind, | ||
| Function<T, R> projectionImplementation) { | ||
| super(resultKind, List.of(argumentKind)); | ||
| this.projectionImplementation = projectionImplementation; | ||
| } | ||
|
|
||
| @Override | ||
| @SuppressWarnings("unchecked") | ||
| protected R doUnwrappedProjection(List<Object> arguments) { | ||
| return this.projectionImplementation.apply((T) arguments.get(0)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious, where do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the default function to try to focus on getting a POC with a single function first - left it in because we do want this functionality and I already had tested it, just didn't finish hooking it up. As an example of where this might be used, take the case where we're generating service name off the serviceName tag if provided, else something else. Our own elvis operator.