From 12727e216570537dcb42fbaeb6e801d31b8fe435 Mon Sep 17 00:00:00 2001 From: James Kebinger Date: Fri, 2 Jun 2023 11:52:25 -0500 Subject: [PATCH 1/7] Add some more logging in the keys and shape aggregators --- .../prefab/client/internal/ContextShapeAggregator.java | 5 +++-- .../prefab/client/internal/EvaluatedKeysAggregator.java | 6 ++++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/client/src/main/java/cloud/prefab/client/internal/ContextShapeAggregator.java b/client/src/main/java/cloud/prefab/client/internal/ContextShapeAggregator.java index 25909e92..033ba5cf 100644 --- a/client/src/main/java/cloud/prefab/client/internal/ContextShapeAggregator.java +++ b/client/src/main/java/cloud/prefab/client/internal/ContextShapeAggregator.java @@ -75,8 +75,9 @@ void start() { @VisibleForTesting void doUpload() { if (shouldUpload()) { - LOG.debug("uploading context shapes"); - prefabHttpClient.reportContextShape(buildProtoShapesFromShapeState()); + Prefab.ContextShapes shapesToUpload = buildProtoShapesFromShapeState(); + LOG.debug("uploading context shapes {}", shapesToUpload); + prefabHttpClient.reportContextShape(shapesToUpload); lastUploadTime = clock.millis(); dirtyFlag.set(false); } diff --git a/client/src/main/java/cloud/prefab/client/internal/EvaluatedKeysAggregator.java b/client/src/main/java/cloud/prefab/client/internal/EvaluatedKeysAggregator.java index 71b34680..cc5de53d 100644 --- a/client/src/main/java/cloud/prefab/client/internal/EvaluatedKeysAggregator.java +++ b/client/src/main/java/cloud/prefab/client/internal/EvaluatedKeysAggregator.java @@ -60,7 +60,7 @@ void start() { try { doUpload(); } catch (Exception e) { - LOG.debug("error uploading context shapes", e); + LOG.debug("error uploading evaluated keys", e); } }, 1, @@ -76,7 +76,9 @@ void doUpload() { .newBuilder() .addAllKeys(evaluatedKeys); namespace.ifPresent(builder::setNamespace); - prefabHttpClient.reportEvaluatedKeys(builder.build()); + Prefab.EvaluatedKeys keysToUpload = builder.build(); + LOG.debug("uploading evaluated keys {}", keysToUpload); + prefabHttpClient.reportEvaluatedKeys(keysToUpload); lastUploadTime = clock.millis(); dirtyFlag.set(false); } From 3ecaebf10c488a200a6c79e82590c1c4ecf7f135 Mon Sep 17 00:00:00 2001 From: James Kebinger Date: Fri, 2 Jun 2023 12:12:56 -0500 Subject: [PATCH 2/7] Add toStrings for Contexts --- .../cloud/prefab/context/PrefabContext.java | 19 ++++++++++++++----- .../prefab/context/PrefabContextSet.java | 8 ++++++++ 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/client/src/main/java/cloud/prefab/context/PrefabContext.java b/client/src/main/java/cloud/prefab/context/PrefabContext.java index 8da7cb0f..b84d5155 100644 --- a/client/src/main/java/cloud/prefab/context/PrefabContext.java +++ b/client/src/main/java/cloud/prefab/context/PrefabContext.java @@ -59,6 +59,20 @@ public boolean equals(Object o) { ); } + @Override + public int hashCode() { + return Objects.hash(name, properties); + } + + @Override + public String toString() { + return com.google.common.base.MoreObjects + .toStringHelper(this) + .add("name", name) + .add("properties", properties) + .toString(); + } + public Prefab.ContextShape getShape() { Prefab.ContextShape.Builder shapeBuilder = Prefab.ContextShape .newBuilder() @@ -69,11 +83,6 @@ public Prefab.ContextShape getShape() { return shapeBuilder.build(); } - @Override - public int hashCode() { - return Objects.hash(name, properties); - } - public static PrefabContext unnamedFromMap( Map configValueMap ) { diff --git a/client/src/main/java/cloud/prefab/context/PrefabContextSet.java b/client/src/main/java/cloud/prefab/context/PrefabContextSet.java index 43fac1d5..57d9b525 100644 --- a/client/src/main/java/cloud/prefab/context/PrefabContextSet.java +++ b/client/src/main/java/cloud/prefab/context/PrefabContextSet.java @@ -73,4 +73,12 @@ public boolean equals(Object o) { public int hashCode() { return Objects.hash(contextByNameMap); } + + @Override + public String toString() { + return com.google.common.base.MoreObjects + .toStringHelper(this) + .add("contextByNameMap", contextByNameMap) + .toString(); + } } From bfb15231c57bec01f6baae3387f9e9481dd2bef3 Mon Sep 17 00:00:00 2001 From: James Kebinger Date: Fri, 2 Jun 2023 15:40:48 -0500 Subject: [PATCH 3/7] Implement isAvailable here so that ServerRequestContextStore properly rules itself out for eg GRPC threads --- .../prefab/client/micronaut/ServerRequestContextStore.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/micronaut/src/main/java/cloud/prefab/client/micronaut/ServerRequestContextStore.java b/micronaut/src/main/java/cloud/prefab/client/micronaut/ServerRequestContextStore.java index 6b2b6a7b..402a2d99 100644 --- a/micronaut/src/main/java/cloud/prefab/client/micronaut/ServerRequestContextStore.java +++ b/micronaut/src/main/java/cloud/prefab/client/micronaut/ServerRequestContextStore.java @@ -65,4 +65,9 @@ private Optional getPrefabContextSet() { objectHttpRequest.getAttribute(ATTRIBUTE_NAME, PrefabContextSet.class) ); } + + @Override + public boolean isAvailable() { + return ServerRequestContext.currentRequest().isPresent(); + } } From ac5d91928220fd33f495a8db33b208e06204d006 Mon Sep 17 00:00:00 2001 From: James Kebinger Date: Fri, 2 Jun 2023 15:41:08 -0500 Subject: [PATCH 4/7] Return success for reporting context shape, evaluated keys --- .../client/internal/PrefabHttpClient.java | 34 +++++++++++-------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/client/src/main/java/cloud/prefab/client/internal/PrefabHttpClient.java b/client/src/main/java/cloud/prefab/client/internal/PrefabHttpClient.java index dbb7f376..5ed2c8b0 100644 --- a/client/src/main/java/cloud/prefab/client/internal/PrefabHttpClient.java +++ b/client/src/main/java/cloud/prefab/client/internal/PrefabHttpClient.java @@ -69,7 +69,7 @@ void reportLoggers(Prefab.Loggers loggers) { } } - void reportContextShape(Prefab.ContextShapes contextShapes) { + boolean reportContextShape(Prefab.ContextShapes contextShapes) { HttpRequest request = getClientBuilderWithStandardHeaders() .header("Content-Type", PROTO_MEDIA_TYPE) .header("Accept", PROTO_MEDIA_TYPE) @@ -82,23 +82,25 @@ void reportContextShape(Prefab.ContextShapes contextShapes) { request, HttpResponse.BodyHandlers.ofString() ); - - if (!isSuccess(response.statusCode())) { - LOG.info( - "Uploading context shapes returned unsuccessful code {} with body {}", - response.statusCode(), - response.body() - ); + if (isSuccess(response.statusCode())) { + return true; } + + LOG.info( + "Uploading context shapes returned unsuccessful code {} with body {}", + response.statusCode(), + response.body() + ); } catch (IOException e) { LOG.warn("Error uploading context shapes via http {}", e.getMessage()); } catch (InterruptedException e) { LOG.warn("Interrupted while uploading context shapes"); Thread.currentThread().interrupt(); } + return false; } - void reportEvaluatedKeys(Prefab.EvaluatedKeys evaluatedKeys) { + boolean reportEvaluatedKeys(Prefab.EvaluatedKeys evaluatedKeys) { HttpRequest request = getClientBuilderWithStandardHeaders() .header("Content-Type", PROTO_MEDIA_TYPE) .header("Accept", PROTO_MEDIA_TYPE) @@ -112,19 +114,21 @@ void reportEvaluatedKeys(Prefab.EvaluatedKeys evaluatedKeys) { HttpResponse.BodyHandlers.ofString() ); - if (!isSuccess(response.statusCode())) { - LOG.info( - "Uploading evaluated keys returned unsuccessful code {} with body {}", - response.statusCode(), - response.body() - ); + if (isSuccess(response.statusCode())) { + return true; } + LOG.info( + "Uploading evaluated keys returned unsuccessful code {} with body {}", + response.statusCode(), + response.body() + ); } catch (IOException e) { LOG.warn("Error uploading evaluated keys via http {}", e.getMessage()); } catch (InterruptedException e) { LOG.warn("Interrupted while uploading evaluated keys"); Thread.currentThread().interrupt(); } + return false; } CompletableFuture> requestConfigSSE( From 8d05fc86904b5a4a44cbfc9dcaf33501f0bed86f Mon Sep 17 00:00:00 2001 From: James Kebinger Date: Fri, 2 Jun 2023 15:43:29 -0500 Subject: [PATCH 5/7] Check success of upload in shape/eval-key aggregators --- .../prefab/client/internal/ContextShapeAggregator.java | 9 ++++++--- .../prefab/client/internal/EvaluatedKeysAggregator.java | 7 ++++--- .../client/internal/ContextShapeAggregatorTest.java | 2 ++ .../client/internal/EvaluatedKeysAggregatorTest.java | 2 ++ 4 files changed, 14 insertions(+), 6 deletions(-) diff --git a/client/src/main/java/cloud/prefab/client/internal/ContextShapeAggregator.java b/client/src/main/java/cloud/prefab/client/internal/ContextShapeAggregator.java index 033ba5cf..d77b56eb 100644 --- a/client/src/main/java/cloud/prefab/client/internal/ContextShapeAggregator.java +++ b/client/src/main/java/cloud/prefab/client/internal/ContextShapeAggregator.java @@ -75,11 +75,14 @@ void start() { @VisibleForTesting void doUpload() { if (shouldUpload()) { + boolean previousDirtyFlagValue = dirtyFlag.getAndSet(false); Prefab.ContextShapes shapesToUpload = buildProtoShapesFromShapeState(); LOG.debug("uploading context shapes {}", shapesToUpload); - prefabHttpClient.reportContextShape(shapesToUpload); - lastUploadTime = clock.millis(); - dirtyFlag.set(false); + if (prefabHttpClient.reportContextShape(shapesToUpload)) { + lastUploadTime = clock.millis(); + } else { + dirtyFlag.compareAndSet(false, previousDirtyFlagValue); + } } } diff --git a/client/src/main/java/cloud/prefab/client/internal/EvaluatedKeysAggregator.java b/client/src/main/java/cloud/prefab/client/internal/EvaluatedKeysAggregator.java index cc5de53d..474c54c4 100644 --- a/client/src/main/java/cloud/prefab/client/internal/EvaluatedKeysAggregator.java +++ b/client/src/main/java/cloud/prefab/client/internal/EvaluatedKeysAggregator.java @@ -78,9 +78,10 @@ void doUpload() { namespace.ifPresent(builder::setNamespace); Prefab.EvaluatedKeys keysToUpload = builder.build(); LOG.debug("uploading evaluated keys {}", keysToUpload); - prefabHttpClient.reportEvaluatedKeys(keysToUpload); - lastUploadTime = clock.millis(); - dirtyFlag.set(false); + if (prefabHttpClient.reportEvaluatedKeys(keysToUpload)) { + lastUploadTime = clock.millis(); + dirtyFlag.set(false); + } } } diff --git a/client/src/test/java/cloud/prefab/client/internal/ContextShapeAggregatorTest.java b/client/src/test/java/cloud/prefab/client/internal/ContextShapeAggregatorTest.java index 36b8f2e2..d4df4273 100644 --- a/client/src/test/java/cloud/prefab/client/internal/ContextShapeAggregatorTest.java +++ b/client/src/test/java/cloud/prefab/client/internal/ContextShapeAggregatorTest.java @@ -2,6 +2,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; @@ -39,6 +40,7 @@ class ContextShapeAggregatorTest { @BeforeEach void beforeEach() { + lenient().when(prefabHttpClient.reportContextShape(any())).thenReturn(true); aggregator = new ContextShapeAggregator( new Options().setNamespace("the-namespace"), diff --git a/client/src/test/java/cloud/prefab/client/internal/EvaluatedKeysAggregatorTest.java b/client/src/test/java/cloud/prefab/client/internal/EvaluatedKeysAggregatorTest.java index 1f6fc189..40fd9114 100644 --- a/client/src/test/java/cloud/prefab/client/internal/EvaluatedKeysAggregatorTest.java +++ b/client/src/test/java/cloud/prefab/client/internal/EvaluatedKeysAggregatorTest.java @@ -2,6 +2,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; @@ -38,6 +39,7 @@ class EvaluatedKeysAggregatorTest { @BeforeEach void beforeEach() { + lenient().when(prefabHttpClient.reportEvaluatedKeys(any())).thenReturn(true); aggregator = new EvaluatedKeysAggregator( new Options().setNamespace("the-namespace"), From f890556835eef64fefea49597fb05ad5ff6dedda Mon Sep 17 00:00:00 2001 From: James Kebinger Date: Fri, 2 Jun 2023 15:44:54 -0500 Subject: [PATCH 6/7] Implement more ToStrings for special case implementations of PrefabContextSetReadable --- .../prefab/context/PrefabContextSetReadable.java | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/client/src/main/java/cloud/prefab/context/PrefabContextSetReadable.java b/client/src/main/java/cloud/prefab/context/PrefabContextSetReadable.java index 87f1867b..29dd5d32 100644 --- a/client/src/main/java/cloud/prefab/context/PrefabContextSetReadable.java +++ b/client/src/main/java/cloud/prefab/context/PrefabContextSetReadable.java @@ -24,6 +24,14 @@ public Iterable getContexts() { public boolean isEmpty() { return true; } + + @Override + public String toString() { + return com.google.common.base.MoreObjects + .toStringHelper(this) + .add("contextByNameMap", "EMPTY") + .toString(); + } }; static PrefabContextSetReadable readOnlyContextSetView(PrefabContextSet contextSet) { @@ -42,6 +50,11 @@ public Iterable getContexts() { public boolean isEmpty() { return contextSet.isEmpty(); } + + @Override + public String toString() { + return contextSet.toString(); + } }; } } From d8d454511284f43ae8964e2755617b9187e65db8 Mon Sep 17 00:00:00 2001 From: James Kebinger Date: Fri, 2 Jun 2023 15:47:36 -0500 Subject: [PATCH 7/7] Adds logging to spy on context resolution --- .../prefab/client/internal/ConfigClientImpl.java | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/client/src/main/java/cloud/prefab/client/internal/ConfigClientImpl.java b/client/src/main/java/cloud/prefab/client/internal/ConfigClientImpl.java index a271a6ab..e0424279 100644 --- a/client/src/main/java/cloud/prefab/client/internal/ConfigClientImpl.java +++ b/client/src/main/java/cloud/prefab/client/internal/ConfigClientImpl.java @@ -301,6 +301,20 @@ private PrefabContextSetReadable resolveContext( .getContext() .filter(Predicate.not(PrefabContextSetReadable::isEmpty)); + if (LOG.isTraceEnabled()) { + LOG.trace( + "context store contains {}, after filtering {}", + getContextStore().getContext().map(Object::toString).orElse("n/a"), + existingContext.map(Object::toString).orElse("n/a") + ); + + LOG.trace( + "Merging passed context {} with context store {}", + prefabContextSetReadable, + getContextStore().getContext().map(Object::toString).orElse("n/a") + ); + } + if (newContext.isEmpty()) { return existingContext.orElse(PrefabContextSetReadable.EMPTY); } else {