From 9bf6f1b19615f97b68a41705e28d5fa4773b6158 Mon Sep 17 00:00:00 2001 From: p29876 <165825455+p29876@users.noreply.github.com> Date: Fri, 18 Oct 2024 12:48:55 +0000 Subject: [PATCH 1/2] Change OTEL user attribute (#3321) * Change OTEL user attribute standardise other attributes * address comments --- .../gov/gchq/gaffer/commonutil/otel/OtelUtil.java | 6 ++++++ .../main/java/uk/gov/gchq/gaffer/graph/Graph.java | 6 +++--- .../operation/handler/OperationChainHandler.java | 4 ++-- .../gaffer/rest/controller/GremlinController.java | 13 ++++++++++++- .../rest/handler/GremlinWebSocketHandler.java | 7 +++++-- 5 files changed, 28 insertions(+), 8 deletions(-) diff --git a/core/common-util/src/main/java/uk/gov/gchq/gaffer/commonutil/otel/OtelUtil.java b/core/common-util/src/main/java/uk/gov/gchq/gaffer/commonutil/otel/OtelUtil.java index 6d33b65647d..7cf3e93ab11 100644 --- a/core/common-util/src/main/java/uk/gov/gchq/gaffer/commonutil/otel/OtelUtil.java +++ b/core/common-util/src/main/java/uk/gov/gchq/gaffer/commonutil/otel/OtelUtil.java @@ -21,6 +21,12 @@ public final class OtelUtil { + public static final String USER_ATTRIBUTE = "enduser.id"; + public static final String JOB_ID_ATTRIBUTE = "gaffer.jobId"; + public static final String GRAPH_ID_ATTRIBUTE = "gaffer.graphId"; + public static final String VIEW_ATTRIBUTE = "gaffer.view"; + public static final String GREMLIN_QUERY_ATTRIBUTE = "gaffer.gremlin.query"; + private static boolean openTelemetryActive = false; private OtelUtil() { diff --git a/core/graph/src/main/java/uk/gov/gchq/gaffer/graph/Graph.java b/core/graph/src/main/java/uk/gov/gchq/gaffer/graph/Graph.java index 770e06c6fe3..02d616b425a 100644 --- a/core/graph/src/main/java/uk/gov/gchq/gaffer/graph/Graph.java +++ b/core/graph/src/main/java/uk/gov/gchq/gaffer/graph/Graph.java @@ -321,9 +321,9 @@ private GraphResult _execute(final StoreExecuter storeExecuter, final Span span = OtelUtil.startSpan( this.getClass().getName(), "Graph Request: " + clonedOpChain.toOverviewString()); - span.setAttribute("gaffer.graphId", getGraphId()); - span.setAttribute("gaffer.jobId", clonedContext.getJobId()); - span.setAttribute("gaffer.user", clonedContext.getUser().getUserId()); + span.setAttribute(OtelUtil.GRAPH_ID_ATTRIBUTE, getGraphId()); + span.setAttribute(OtelUtil.JOB_ID_ATTRIBUTE, clonedContext.getJobId()); + span.setAttribute(OtelUtil.USER_ATTRIBUTE, clonedContext.getUser().getUserId()); O result = null; // Sets the span to current so parent child spans are auto linked diff --git a/core/store/src/main/java/uk/gov/gchq/gaffer/store/operation/handler/OperationChainHandler.java b/core/store/src/main/java/uk/gov/gchq/gaffer/store/operation/handler/OperationChainHandler.java index 1187cd7a5dc..f866460215e 100644 --- a/core/store/src/main/java/uk/gov/gchq/gaffer/store/operation/handler/OperationChainHandler.java +++ b/core/store/src/main/java/uk/gov/gchq/gaffer/store/operation/handler/OperationChainHandler.java @@ -52,9 +52,9 @@ public OUT doOperation(final OperationChain operationChain, final Context c for (final Operation op : preparedOperationChain.getOperations()) { // OpenTelemetry hooks Span span = OtelUtil.startSpan(this.getClass().getName(), op.getClass().getName()); - span.setAttribute("jobId", context.getJobId()); + span.setAttribute(OtelUtil.JOB_ID_ATTRIBUTE, context.getJobId()); if (op instanceof OperationView && ((OperationView) op).getView() != null) { - span.setAttribute("view", ((OperationView) op).getView().toString()); + span.setAttribute(OtelUtil.VIEW_ATTRIBUTE, ((OperationView) op).getView().toString()); } // Sets the span to current so parent child spans are auto linked diff --git a/rest-api/spring-rest/src/main/java/uk/gov/gchq/gaffer/rest/controller/GremlinController.java b/rest-api/spring-rest/src/main/java/uk/gov/gchq/gaffer/rest/controller/GremlinController.java index 65c5f294952..14f387ffce0 100644 --- a/rest-api/spring-rest/src/main/java/uk/gov/gchq/gaffer/rest/controller/GremlinController.java +++ b/rest-api/spring-rest/src/main/java/uk/gov/gchq/gaffer/rest/controller/GremlinController.java @@ -57,6 +57,7 @@ import uk.gov.gchq.gaffer.rest.factory.spring.AbstractUserFactory; import uk.gov.gchq.gaffer.tinkerpop.GafferPopGraph; import uk.gov.gchq.gaffer.tinkerpop.GafferPopGraphVariables; +import uk.gov.gchq.gaffer.user.User; import uk.gov.gchq.koryphe.tuple.n.Tuple2; import java.io.IOException; @@ -309,7 +310,17 @@ private Tuple2 runGremlinQuery(final String gremlinQuery) { // OpenTelemetry hooks Span span = OtelUtil.startSpan( this.getClass().getName(), "Gremlin Request: " + UUID.nameUUIDFromBytes(gremlinQuery.getBytes(StandardCharsets.UTF_8))); - span.setAttribute("gaffer.gremlin.query", gremlinQuery); + span.setAttribute(OtelUtil.GREMLIN_QUERY_ATTRIBUTE, gremlinQuery); + + User user = ((GafferPopGraphVariables) gafferPopGraph.variables()).getUser(); + String userId; + if (user != null) { + userId = user.getUserId(); + } else { + LOGGER.warn("Could not find Gaffer user for OTEL. Using default."); + userId = "unknownGremlinUser"; + } + span.setAttribute(OtelUtil.USER_ATTRIBUTE, userId); // tuple to hold the result and explain Tuple2 pair = new Tuple2<>(); diff --git a/rest-api/spring-rest/src/main/java/uk/gov/gchq/gaffer/rest/handler/GremlinWebSocketHandler.java b/rest-api/spring-rest/src/main/java/uk/gov/gchq/gaffer/rest/handler/GremlinWebSocketHandler.java index 05e1b9ebb8f..63865214d48 100644 --- a/rest-api/spring-rest/src/main/java/uk/gov/gchq/gaffer/rest/handler/GremlinWebSocketHandler.java +++ b/rest-api/spring-rest/src/main/java/uk/gov/gchq/gaffer/rest/handler/GremlinWebSocketHandler.java @@ -52,6 +52,7 @@ import uk.gov.gchq.gaffer.rest.factory.spring.AbstractUserFactory; import uk.gov.gchq.gaffer.tinkerpop.GafferPopGraph; import uk.gov.gchq.gaffer.tinkerpop.GafferPopGraphVariables; +import uk.gov.gchq.gaffer.user.User; import java.io.IOException; import java.nio.ByteBuffer; @@ -145,14 +146,16 @@ private ResponseMessage handleGremlinRequest(final WebSocketSession session, fin // OpenTelemetry hooks Span span = OtelUtil.startSpan(this.getClass().getName(), "Gremlin Request: " + requestId.toString()); - span.setAttribute("gaffer.gremlin.query", request.getArgs().get(Tokens.ARGS_GREMLIN).toString()); + span.setAttribute(OtelUtil.GREMLIN_QUERY_ATTRIBUTE, request.getArgs().get(Tokens.ARGS_GREMLIN).toString()); // Execute the query try (Scope scope = span.makeCurrent(); GremlinExecutor gremlinExecutor = getGremlinExecutor()) { // Set current headers for potential authorisation then set the user userFactory.setHttpHeaders(session.getHandshakeHeaders()); - graph.variables().set(GafferPopGraphVariables.USER, userFactory.createUser()); + User user = userFactory.createUser(); + graph.variables().set(GafferPopGraphVariables.USER, user); + span.setAttribute(OtelUtil.USER_ATTRIBUTE, user.getUserId()); // Run the query using the gremlin executor service Object result = gremlinExecutor.eval( From a5a67ac67990009ed7dcb28d0728edc4a902f5c6 Mon Sep 17 00:00:00 2001 From: cn337131 <141730190+cn337131@users.noreply.github.com> Date: Mon, 21 Oct 2024 11:44:06 +0000 Subject: [PATCH 2/2] Gh-3319: Fixes ability to get orphan vertices from edges (#3320) * bug fix, tests and schema fix * checkstyle fixes * typos --------- Co-authored-by: wb36499 <166839644+wb36499@users.noreply.github.com> --- .../gchq/gaffer/tinkerpop/GafferPopGraph.java | 42 +++++++++++++------ .../traversal/util/GafferVertexUtils.java | 28 +++++++------ .../gaffer/tinkerpop/GafferPopGraphIT.java | 42 +++++++++++++------ 3 files changed, 76 insertions(+), 36 deletions(-) diff --git a/library/tinkerpop/src/main/java/uk/gov/gchq/gaffer/tinkerpop/GafferPopGraph.java b/library/tinkerpop/src/main/java/uk/gov/gchq/gaffer/tinkerpop/GafferPopGraph.java index 593cd088205..e416951b825 100755 --- a/library/tinkerpop/src/main/java/uk/gov/gchq/gaffer/tinkerpop/GafferPopGraph.java +++ b/library/tinkerpop/src/main/java/uk/gov/gchq/gaffer/tinkerpop/GafferPopGraph.java @@ -35,6 +35,7 @@ import org.slf4j.LoggerFactory; import uk.gov.gchq.gaffer.data.element.Element; +import uk.gov.gchq.gaffer.data.element.id.EntityId; import uk.gov.gchq.gaffer.data.elementdefinition.view.View; import uk.gov.gchq.gaffer.graph.Graph; import uk.gov.gchq.gaffer.graph.GraphConfig; @@ -51,6 +52,7 @@ import uk.gov.gchq.gaffer.operation.impl.get.GetAllElements; import uk.gov.gchq.gaffer.operation.impl.get.GetElements; import uk.gov.gchq.gaffer.operation.io.Input; +import uk.gov.gchq.gaffer.store.operation.GetSchema; import uk.gov.gchq.gaffer.store.schema.Schema; import uk.gov.gchq.gaffer.tinkerpop.generator.GafferEdgeGenerator; import uk.gov.gchq.gaffer.tinkerpop.generator.GafferEntityGenerator; @@ -80,6 +82,7 @@ import java.util.concurrent.atomic.AtomicLong; import java.util.regex.Matcher; import java.util.regex.Pattern; +import java.util.stream.Collectors; import java.util.stream.Stream; import java.util.stream.StreamSupport; @@ -540,7 +543,6 @@ public Iterator adjVerticesWithView(final Object vertexId, final Directi * Given an iterable of vertex ids, adjacent vertices will be returned. * If you provide any optional labels then you must provide edge labels and the vertex * labels - any missing labels will cause the elements to be filtered out. - * This method will not return 'id' vertices, only vertices that exist as entities in Gaffer. * * @param vertexIds the iterable of vertex ids to start at. * @param direction the direction along edges to travel @@ -830,16 +832,21 @@ private Iterator adjVerticesWithSeedsAndView(final List see throw new UnsupportedOperationException("There could be a lot of vertices, so please add some seeds"); } - final Iterable result = execute(new OperationChain.Builder() + final Iterable getAdjEntitySeeds = execute(new OperationChain.Builder() .first(new GetAdjacentIds.Builder() - .input(seeds) - .view(view) - .inOutType(getInOutType(direction)) - .build()) - // GetAdjacentIds provides list of entity seeds so run a GetElements to get the actual Entities - .then(new GetElements.Builder() - .view(createAllEntitiesView()) - .build()) + .input(seeds) + .view(view) + .inOutType(getInOutType(direction)) + .build()) + .build()); + + List seedList = StreamSupport.stream(getAdjEntitySeeds.spliterator(), false).collect(Collectors.toList()); + + // GetAdjacentIds provides list of entity seeds so run a GetElements to get the actual Entities + final Iterable result = execute(new OperationChain.Builder() + .first(new GetElements.Builder() + .input(seedList) + .build()) .build()); // Translate results to Gafferpop elements @@ -850,7 +857,13 @@ private Iterator adjVerticesWithSeedsAndView(final List see .map(e -> (Vertex) e) .iterator(); - return translatedResults.iterator(); + // Check for seeds that are not entities but are vertices on an edge (orphan vertices) + Iterable chainedIterable = translatedResults; + for (final EntityId seed : seedList) { + Iterable orphanVertices = GafferVertexUtils.getOrphanVertices(result, this, seed.getVertex()); + chainedIterable = IterableUtils.chainedIterable(chainedIterable, orphanVertices); + } + return chainedIterable.iterator(); } private Iterator edgesWithSeedsAndView(final List seeds, final Direction direction, final View view) { @@ -922,7 +935,12 @@ private View createView(final String... labels) { View view = null; if (null != labels && 0 < labels.length) { final View.Builder viewBuilder = new View.Builder(); - final Schema schema = graph.getSchema(); + final Schema schema = execute(new OperationChain.Builder() + .first(new GetSchema.Builder() + .compact(true) + .build()) + .options(opOptions) + .build()); for (final String label : labels) { if (schema.isEntity(label)) { viewBuilder.entity(label); diff --git a/library/tinkerpop/src/main/java/uk/gov/gchq/gaffer/tinkerpop/process/traversal/util/GafferVertexUtils.java b/library/tinkerpop/src/main/java/uk/gov/gchq/gaffer/tinkerpop/process/traversal/util/GafferVertexUtils.java index bdef1c4295c..be409317801 100644 --- a/library/tinkerpop/src/main/java/uk/gov/gchq/gaffer/tinkerpop/process/traversal/util/GafferVertexUtils.java +++ b/library/tinkerpop/src/main/java/uk/gov/gchq/gaffer/tinkerpop/process/traversal/util/GafferVertexUtils.java @@ -18,20 +18,24 @@ import org.apache.tinkerpop.gremlin.structure.Vertex; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + import uk.gov.gchq.gaffer.data.element.Edge; import uk.gov.gchq.gaffer.data.element.Element; import uk.gov.gchq.gaffer.data.element.Entity; import uk.gov.gchq.gaffer.tinkerpop.GafferPopGraph; import uk.gov.gchq.gaffer.tinkerpop.GafferPopVertex; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.List; -import java.util.Objects; import java.util.stream.Collectors; import java.util.stream.StreamSupport; public final class GafferVertexUtils { + private static final Logger LOGGER = LoggerFactory.getLogger(GafferVertexUtils.class); private GafferVertexUtils() { // Utility class @@ -58,6 +62,7 @@ public static Iterable getOrphanVertices(final Iterable e.equals(id))) .collect(Collectors.toList()); + orphanVertexIds.forEach(id -> LOGGER.debug("Getting orphan vertices for vertex {}", id)); return (orphanVertexIds.isEmpty()) ? Collections.emptyList() : extractOrphanVerticesFromEdges(result, graph, orphanVertexIds); } @@ -71,19 +76,18 @@ public static Iterable getOrphanVertices(final Iterable extractOrphanVerticesFromEdges(final Iterable result, final GafferPopGraph graph, final List orphanVertexIds) { - return StreamSupport.stream(result.spliterator(), false) + List orphanVertices = new ArrayList<>(); + StreamSupport.stream(result.spliterator(), false) .filter(Edge.class::isInstance) .map(e -> (Edge) e) - .map(e -> { - if (orphanVertexIds.contains(e.getSource())) { - return new GafferPopVertex(GafferPopGraph.ID_LABEL, GafferCustomTypeFactory.parseForGraphSONv3(e.getSource()), graph); - } else if (orphanVertexIds.contains(e.getDestination())) { - return new GafferPopVertex(GafferPopGraph.ID_LABEL, GafferCustomTypeFactory.parseForGraphSONv3(e.getDestination()), graph); - } else { - return null; + .forEach(e -> { + if (orphanVertexIds.contains(e.getSource()) || orphanVertexIds.equals(e.getSource())) { + orphanVertices.add(new GafferPopVertex(GafferPopGraph.ID_LABEL, GafferCustomTypeFactory.parseForGraphSONv3(e.getSource()), graph)); } - }) - .filter(Objects::nonNull) - .collect(Collectors.toList()); + if (orphanVertexIds.contains(e.getDestination()) || orphanVertexIds.equals(e.getDestination())) { + orphanVertices.add(new GafferPopVertex(GafferPopGraph.ID_LABEL, GafferCustomTypeFactory.parseForGraphSONv3(e.getDestination()), graph)); + } + }); + return orphanVertices; } } diff --git a/library/tinkerpop/src/test/java/uk/gov/gchq/gaffer/tinkerpop/GafferPopGraphIT.java b/library/tinkerpop/src/test/java/uk/gov/gchq/gaffer/tinkerpop/GafferPopGraphIT.java index 341067f09e1..07701d92a76 100644 --- a/library/tinkerpop/src/test/java/uk/gov/gchq/gaffer/tinkerpop/GafferPopGraphIT.java +++ b/library/tinkerpop/src/test/java/uk/gov/gchq/gaffer/tinkerpop/GafferPopGraphIT.java @@ -54,6 +54,7 @@ */ class GafferPopGraphIT { private static final String TEST_NAME_FORMAT = "({0}) {displayName}"; + private static final String VERTEX_ONLY_ID_STRING = "7"; private static GafferPopGraph mapStore; private static GafferPopGraph accumuloStore; @@ -303,28 +304,45 @@ void shouldAddE(String graph, GraphTraversalSource g) { @MethodSource("provideTraversals") void shouldSeedWithVertexOnlyEdge(String graph, GraphTraversalSource g) { // Edge has a vertex but not an entity in the graph - Gaffer only feature - String vertexOnlyId = "7"; - mapStore.addEdge(new GafferPopEdge("knows", GafferPopModernTestUtils.MARKO.getId(), vertexOnlyId, mapStore)); - accumuloStore.addEdge(new GafferPopEdge("knows", GafferPopModernTestUtils.MARKO.getId(), vertexOnlyId, accumuloStore)); + // [1 - knows -> 7] + mapStore.addEdge(new GafferPopEdge("knows", GafferPopModernTestUtils.MARKO.getId(), VERTEX_ONLY_ID_STRING, mapStore)); + accumuloStore.addEdge(new GafferPopEdge("knows", GafferPopModernTestUtils.MARKO.getId(), VERTEX_ONLY_ID_STRING, accumuloStore)); - List result = g.V("7").toList(); + List result = g.V(VERTEX_ONLY_ID_STRING).toList(); assertThat(result) .extracting(r -> r.id()) - .contains(vertexOnlyId); + .contains(VERTEX_ONLY_ID_STRING); reset(); } @ParameterizedTest(name = TEST_NAME_FORMAT) @MethodSource("provideTraversals") - void shouldTraverseEdgeWithVertexOnlySeed(String graph, GraphTraversalSource g) { - // Edge has a vertex but not an entity in the graph - Gaffer only feature - String vertexOnlyId = "7"; - mapStore.addEdge(new GafferPopEdge("knows", GafferPopModernTestUtils.MARKO.getId(), vertexOnlyId, mapStore)); - accumuloStore.addEdge(new GafferPopEdge("knows", GafferPopModernTestUtils.MARKO.getId(), vertexOnlyId, accumuloStore)); + void shouldTraverseEdgeWithVertexOnlyEdge(String graph, GraphTraversalSource g) { + // Edge has a two vertices with no entities in the graph - Gaffer only feature + // [8 - knows -> 7] + mapStore.addEdge(new GafferPopEdge("knows", "8", VERTEX_ONLY_ID_STRING, mapStore)); + accumuloStore.addEdge(new GafferPopEdge("knows", "8", VERTEX_ONLY_ID_STRING, accumuloStore)); - List> result = g.V("7").inE().outV().elementMap().toList(); + List result = g.V(VERTEX_ONLY_ID_STRING).inE().inV().toList(); assertThat(result) - .containsExactly(MARKO.getPropertyMap()); + .extracting(r -> r.id()) + .contains(VERTEX_ONLY_ID_STRING); + List result2 = g.V(VERTEX_ONLY_ID_STRING).inE().outV().toList(); + assertThat(result2) + .extracting(r -> r.id()) + .contains("8"); + List result3 = g.V("8").outE().inV().toList(); + assertThat(result3) + .extracting(r -> r.id()) + .contains(VERTEX_ONLY_ID_STRING); + List result4 = g.V("8").outE().outV().toList(); + assertThat(result4) + .extracting(r -> r.id()) + .contains("8"); + List resultLabel = g.V("8").out("knows").toList(); + assertThat(resultLabel) + .extracting(r -> r.id()) + .containsOnly(VERTEX_ONLY_ID_STRING); reset(); }