Refactoring event logs #821

attilakreiner · 2024-02-29T16:00:00Z

Description

This change refactors the event definitions and introduces *EventFormatter classes.

jfallows · 2024-02-29T18:40:06Z

runtime/engine/src/main/java/io/aklivity/zilla/runtime/engine/event/EventFormatterSpi.java

+        DirectBuffer buffer,
+        int index,
+        int length);
+}


Please add

public final class EventFomatter { public static EventFomatter instantiate() { ... use service loader to discover event formatter spis and return an event formatter that can delegate } public String format( int msgTypeId, DirectBuffer buffer, int index, int length) { ... lookup event formatter spi by type id and return formatted event } }

jfallows · 2024-02-29T18:40:54Z

runtime/engine/src/main/java/io/aklivity/zilla/runtime/engine/event/EventFormatterSpi.java

+    String formatEventEx(
+        int msgTypeId,
+        DirectBuffer buffer,
+        int index,
+        int length);


Suggested change

String formatEventEx(

int msgTypeId,

DirectBuffer buffer,

int index,

int length);

String format(

DirectBuffer buffer,

int index,

int length);

We already advertised our required type, so we don't need to pass it again here.

jfallows · 2024-02-29T19:23:06Z

...c/main/java/io/aklivity/zilla/runtime/exporter/stdout/internal/StdoutExporterFactorySpi.java

+        Map<String, EventFormatterSpi> formatters = ServiceLoader.load(EventFormatterSpi.class)
+            .stream()
+            .collect(Collectors.toMap(
+                provider -> provider.get().type(),
+                provider -> provider.get()
+            ));


Please move this service loader code to the engine, exposed as an EventFormatter obtained from EngineContext.

See comments on EventFormatterSpi for more details.

(normally we don't expose the spis outside of the engine except as interfaces for implementation classes)

jfallows · 2024-02-29T19:24:08Z

.../src/main/java/io/aklivity/zilla/runtime/exporter/stdout/internal/StdoutExporterContext.java

@@ -48,7 +53,7 @@ public ExporterHandler attach(
        LongFunction<KindConfig> resolveKind)
    {
        StdoutExporterConfig stdoutExporter = new StdoutExporterConfig(exporter);
-        return new StdoutExporterHandler(config, context, stdoutExporter);
+        return new StdoutExporterHandler(config, formatters, context, stdoutExporter);


Suggested change

return new StdoutExporterHandler(config, formatters, context, stdoutExporter);

return new StdoutExporterHandler(config, formatter, context, stdoutExporter);

where formatter is an EventFormatter provided by the engine

jfallows · 2024-02-29T19:39:43Z

.../main/java/io/aklivity/zilla/runtime/exporter/stdout/internal/stream/StdoutEventsStream.java

+    private static final String FORMAT = "%s [%s] %s%n"; // qname [timestamp] extension\n
+    private static final DateTimeFormatter FORMATTER = DateTimeFormatter.ofPattern("dd/MMM/yyyy:HH:mm:ss Z");


Prefer we use a single format and avoid the supplementary date time formatter.
See https://stackoverflow.com/a/61502608

Suggested change

private static final String FORMAT = "%s [%s] %s%n"; // qname [timestamp] extension\n

private static final DateTimeFormatter FORMATTER = DateTimeFormatter.ofPattern("dd/MMM/yyyy:HH:mm:ss Z");

private static final String FORMAT = "%s [%2$tD:%2$tT] %s%n"; // qname [timestamp] extension\n

jfallows · 2024-02-29T19:43:30Z

...t/src/test/java/io/aklivity/zilla/runtime/exporter/stdout/internal/events/Http2EventsIT.java

@@ -63,6 +63,6 @@ public class Http2EventsIT
    public void connectionHeaders() throws Exception
    {
        k3po.finish();
-        output.expect(Pattern.compile("test.net0 - \\[[^\\]]+\\] REQUEST_ACCEPTED http GET localhost:8080 /\n"));
+        output.expect(Pattern.compile("ephemeral.net0 \\[[^\\]]+\\] REQUEST_ACCEPTED - http GET localhost:8080 /\n"));


This seems to be the wrong binding id on the event.

We need to change this test to use test binding and break dependency on http binding, then additional type-specific tests in the stdout exporter are no longer needed.

jfallows · 2024-02-29T19:50:35Z

...g-http/src/main/java/io/aklivity/zilla/runtime/binding/http/internal/HttpEventFormatter.java

+import io.aklivity.zilla.runtime.binding.http.internal.types.event.HttpRequestAcceptedExFW;
+import io.aklivity.zilla.runtime.engine.event.EventFormatterSpi;
+
+public class HttpEventFormatter implements EventFormatterSpi


Given that formatters are not thread-safe, we'll need to switch up the spi strategy to use an EventFormatterFactorySpi that creates an EventFormatterSpi when needed to instantiate an EventFormatter per worker, instead of one instance of each EventFormatterSpi shared across all workers.

jfallows · 2024-02-29T19:57:00Z

...tp/src/test/java/io/aklivity/zilla/runtime/binding/http/internal/HttpEventFormatterTest.java

+import io.aklivity.zilla.runtime.binding.http.internal.types.event.EventFW;
+import io.aklivity.zilla.runtime.binding.http.internal.types.event.HttpEventExFW;
+
+public class HttpEventFormatterTest


I realize you prefer to write this as a unit test, but now I think we don't actually have an IT that proves the event is written when the scenario occurs, say like authorization failed.

Suggest we instead convert these unit tests to ITs and configure test exporter to have a list of expected events, so we can verify binding id, formatted event text and throw an exception if they don't match so the IT will fail.

That approach should give us good clarity in the scenarios both that the event has occurred and how it is formatted.

This feedback applies to all event formatter unit tests -> ITs.

jfallows · 2024-02-29T20:06:58Z

...afka/src/main/java/io/aklivity/zilla/runtime/binding/kafka/internal/KafkaEventFormatter.java

+        {
+        case AUTHORIZATION_FAILED:
+        {
+            result = AUTHORIZATION_FAILED_FORMAT;


Suspect we'd like to have attempted identity here, much like guard auth failed, agree?

jfallows · 2024-02-29T20:12:10Z

specs/engine.spec/src/main/resources/META-INF/zilla/core.idl

@@ -108,6 +108,7 @@ scope core
            int64 timestamp;
            int64 traceId;
            int64 namespacedId;
+            octets extension;


We do this in frames because extension is optional, but it is required for events (agree?) so perhaps it would be more convenient to put Extension as the type here instead of octets?

jfallows

Looking good so far, needs the ITs now instead of the Tests.

jfallows

Looks good so far.

Make sure to add tls client tests, and verify minimal dependencies on stdout exporter pom now that we only need the engine apis and test binding with test events.

jfallows · 2024-03-05T16:57:21Z

...nding-tls/src/test/java/io/aklivity/zilla/runtime/binding/tls/internal/streams/ClientIT.java

+    @Specification({
+        "${app}/client.handshake.timeout/client",
+        "${net}/client.handshake.timeout/server" })
+    @Configure(name = "zilla.binding.tls.handshake.timeout", value = "1")


These strings should be constants on TlsConfigurationTest, and TlsConfigurationTest test method should verify that the constant string matches the config property name.

Please apply the same feedback about constants (plus tests) to any new tests you've added that use @Configure.

jfallows · 2024-03-05T17:24:24Z

...klivity/zilla/runtime/catalog/schema/registry/internal/SchemaRegistryEventFormatterTest.java

+        // THEN
+        assertThat(result, equalTo("REMOTE_ACCESS_REJECTED GET http://localhost:8080/hello 500"));
+    }
+}


This should be replaced by an IT, right?

jfallows · 2024-03-05T17:32:37Z

.../io/aklivity/zilla/runtime/engine/test/internal/binding/config/TestBindingOptionsConfig.java

 import java.util.function.Function;

 import io.aklivity.zilla.runtime.engine.config.OptionsConfig;

 public final class TestBindingOptionsConfig extends OptionsConfig
 {
    public final String mode;
+    public final List<String> catalogs;
+    public final List<Guard> guards;


Let's change guards of type List<Guard> to authorization (one, not list) of type TestAuthorizationConfig with a String name (of the referenced guard) and credentials (the token). This is more or less how we do it for HTTP binding with HttpAuthorizationConfig except that one is extracting the token from a patterned string, and here we can just specify the token directly instead.

The syntax in yaml should also follow the same approach as other binding authorization syntax.

bindings: test0: type: test kind: server options: authorization: jwt0: credentials: token

jfallows · 2024-03-05T17:34:17Z

...rd-jwt/src/test/java/io/aklivity/zilla/runtime/guard/jwt/internal/JwtEventFormatterTest.java

+        // THEN
+        assertThat(result, equalTo("AUTHORIZATION_FAILED user"));
+    }
+}


Can be removed as we have an IT instead that exercises the event formatter via declarative test.

Please remove all the other event formatter tests as they are also covered by their ITs instead.

jfallows · 2024-03-05T17:35:30Z

...e.spec/src/main/scripts/io/aklivity/zilla/specs/engine/schema/binding/test.schema.patch.json

+                            "guards":
+                            {
+                                "type": "array",
+                                "items":
+                                {
+                                    "type": "object",
+                                    "properties":
+                                    {
+                                        "guard":
+                                        {
+                                            "type": "string"
+                                        },
+                                        "token":
+                                        {
+                                            "type": "string"
+                                        }
+                                    }
+                                }
+                            },


Please change to authorization with patternProperties and credentials as described in other feedback comment.

jfallows · 2024-03-05T17:36:54Z

specs/guard-jwt.spec/src/main/scripts/io/aklivity/zilla/specs/guard/jwt/config/event.yaml

+bindings:
+  net0:
+    type: test
+    kind: server
+    options:
+      guards:
+        - guard: jwt0
+          token: eyJ0eXAiOiJKV1QiLCJhbGciOiJSUzI1NiIsImtpZCI6ImV4YW1wbGUifQ.eyJhdWQiOiJodHRwczovL2FwaS5leGFtcGxlLmNvbSIsImV4cCI6MTcwODk0MDQ0MCwiaXNzIjoiaHR0cHM6Ly9hdXRoLmV4YW1wbGUuY29tIiwic3ViIjoidXNlciJ9.KwZt_rDTDEOQ33URwZ8Gd1WqQjAIJESbt5Et309Yz7AJm1wWWyFhWm7AV6lt1rkX-eD-jVh0wHAsy7L4kdzBOErCdwFcdaB4u2jFDGn_9IK28lCedxIYmYtI4qn6eY916IIqRpwZcqzw08OEljfYUKo4UeX7JPAtha0GQmfZY1-NNcncg06xw3xkKSZ1SnIh9MZM1FNH_5QPZPL4NHP7DRXtaMn2w6YpO7n695Sc_3LuSDlfDDMVIUWuEreOzOXem6jheGIbJ-eDKhIXXPrOz1NBAJmvizbugMR7m_bodiEzzqt5ttKDs1974alrR_sYP8OYmR_rCqXc5N3lp_SW3A
+    exit: app0


Suggested change

bindings:

net0:

type: test

kind: server

options:

guards:

- guard: jwt0

token: eyJ0eXAiOiJKV1QiLCJhbGciOiJSUzI1NiIsImtpZCI6ImV4YW1wbGUifQ.eyJhdWQiOiJodHRwczovL2FwaS5leGFtcGxlLmNvbSIsImV4cCI6MTcwODk0MDQ0MCwiaXNzIjoiaHR0cHM6Ly9hdXRoLmV4YW1wbGUuY29tIiwic3ViIjoidXNlciJ9.KwZt_rDTDEOQ33URwZ8Gd1WqQjAIJESbt5Et309Yz7AJm1wWWyFhWm7AV6lt1rkX-eD-jVh0wHAsy7L4kdzBOErCdwFcdaB4u2jFDGn_9IK28lCedxIYmYtI4qn6eY916IIqRpwZcqzw08OEljfYUKo4UeX7JPAtha0GQmfZY1-NNcncg06xw3xkKSZ1SnIh9MZM1FNH_5QPZPL4NHP7DRXtaMn2w6YpO7n695Sc_3LuSDlfDDMVIUWuEreOzOXem6jheGIbJ-eDKhIXXPrOz1NBAJmvizbugMR7m_bodiEzzqt5ttKDs1974alrR_sYP8OYmR_rCqXc5N3lp_SW3A

exit: app0

bindings:

net0:

type: test

kind: server

options:

authorization:

jwt0:

credentials: eyJ0eXAiOiJKV1QiLCJhbGciOiJSUzI1NiIsImtpZCI6ImV4YW1wbGUifQ.eyJhdWQiOiJodHRwczovL2FwaS5leGFtcGxlLmNvbSIsImV4cCI6MTcwODk0MDQ0MCwiaXNzIjoiaHR0cHM6Ly9hdXRoLmV4YW1wbGUuY29tIiwic3ViIjoidXNlciJ9.KwZt_rDTDEOQ33URwZ8Gd1WqQjAIJESbt5Et309Yz7AJm1wWWyFhWm7AV6lt1rkX-eD-jVh0wHAsy7L4kdzBOErCdwFcdaB4u2jFDGn_9IK28lCedxIYmYtI4qn6eY916IIqRpwZcqzw08OEljfYUKo4UeX7JPAtha0GQmfZY1-NNcncg06xw3xkKSZ1SnIh9MZM1FNH_5QPZPL4NHP7DRXtaMn2w6YpO7n695Sc_3LuSDlfDDMVIUWuEreOzOXem6jheGIbJ-eDKhIXXPrOz1NBAJmvizbugMR7m_bodiEzzqt5ttKDs1974alrR_sYP8OYmR_rCqXc5N3lp_SW3A

exit: app0

attilakreiner added 13 commits February 29, 2024 11:19

WIP http.idl

cfd675e

WIP kafka.idl

b49ded8

WIP tcp.idl

1c31b3a

WIP tls.idl

b7955f1

WIP jwt.idl

68190b3

WIP sch_reg.idl

be0a8ce

fix http originId

924a660

WIP HttpEventFormatter

1d648ef

WIP KafkaEventFormatter

aac1a96

WIP TcpEventFormatter

c374119

WIP TlsEventFormatter

d12a0f7

WIP JwtEventFormatter

6cc773e

WIP SchRegEventFormatter

77aa0ac

attilakreiner requested a review from jfallows February 29, 2024 16:03

jfallows requested changes Feb 29, 2024

View reviewed changes

attilakreiner added 6 commits March 1, 2024 10:16

fix http routedId

2aea525

fix kafka identity

5523659

WIP EventFormatterSpi

d6a126a

WIP EventFormatter

e622e8b

WIP EventFormatterFactorySpi

30768c4

cleanup

d97fecb

jfallows reviewed Mar 4, 2024

View reviewed changes

attilakreiner added 5 commits March 4, 2024 16:23

WIP testing

49229a6

fix engine test

c72f6c5

WIP http EventIT

36e5fe6

fix engine

d2e092c

WIP kafka EventIT

76c134d

jfallows previously approved these changes Mar 4, 2024

View reviewed changes

attilakreiner added 2 commits March 4, 2024 18:24

rm HttpEventFormatterTest

d4c9f2d

rm KafkaEventFormatterTest

fcf8987

attilakreiner added 4 commits March 4, 2024 18:41

WIP tcp EventIT

0c30d4f

WIP tls EventIT

2269d2c

fix

ecdb38e

WIP stdexp EventIT

d6b4ea2

attilakreiner dismissed jfallows’s stale review via d6b4ea2 March 4, 2024 19:46

jfallows reviewed Mar 4, 2024

View reviewed changes

attilakreiner added 5 commits March 5, 2024 08:47

fix stdexp cleanup

2bd852e

WIP schreg EventIT

4ded5e6

WIP jwt EventIT

fd97e79

fix engine.spec

ea473a8

WIP tls IT

548d48e

jfallows requested changes Mar 5, 2024

View reviewed changes

attilakreiner added 4 commits March 5, 2024 18:42

cleanup

eed324c

fix authorization

b2d3126

fix tls

20e94b1

fix stdexp

a498bf1

jfallows approved these changes Mar 5, 2024

View reviewed changes

Merge branch 'develop' into event-ref

c9828f0

jfallows merged commit e966f88 into aklivity:develop Mar 5, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring event logs #821

Refactoring event logs #821

attilakreiner commented Feb 29, 2024

jfallows Feb 29, 2024

jfallows Feb 29, 2024

jfallows Feb 29, 2024

jfallows Feb 29, 2024

jfallows Feb 29, 2024

jfallows Feb 29, 2024

jfallows Feb 29, 2024

jfallows Feb 29, 2024

jfallows Feb 29, 2024

jfallows Feb 29, 2024

jfallows left a comment

jfallows left a comment

jfallows Mar 5, 2024

jfallows Mar 5, 2024

jfallows Mar 5, 2024

jfallows Mar 5, 2024

jfallows Mar 5, 2024

jfallows Mar 5, 2024

	return new StdoutExporterHandler(config, formatters, context, stdoutExporter);
	return new StdoutExporterHandler(config, formatter, context, stdoutExporter);

		private static final String FORMAT = "%s [%s] %s%n"; // qname [timestamp] extension\n
		private static final DateTimeFormatter FORMATTER = DateTimeFormatter.ofPattern("dd/MMM/yyyy:HH:mm:ss Z");

Refactoring event logs #821

Refactoring event logs #821

Conversation

attilakreiner commented Feb 29, 2024

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfallows left a comment

Choose a reason for hiding this comment

jfallows left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment