-
Notifications
You must be signed in to change notification settings - Fork 48
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
feat: replace static event name with dynamic based on event id #1029
feat: replace static event name with dynamic based on event id #1029
Conversation
...ain/java/io/aklivity/zilla/runtime/catalog/filesystem/internal/FilesystemEventFormatter.java
Outdated
Show resolved
Hide resolved
.../main/java/io/aklivity/zilla/runtime/exporter/stdout/internal/stream/StdoutEventsStream.java
Outdated
Show resolved
Hide resolved
...p/src/main/java/io/aklivity/zilla/runtime/exporter/otlp/internal/serializer/EventReader.java
Outdated
Show resolved
Hide resolved
.../src/main/java/io/aklivity/zilla/runtime/exporter/stdout/internal/StdoutExporterContext.java
Outdated
Show resolved
Hide resolved
81138a5
to
fd76938
Compare
LGTM apart from the test failures. It looks like there is test code ( |
...g-kafka.spec/src/main/scripts/io/aklivity/zilla/specs/binding/kafka/config/client.event.yaml
Show resolved
Hide resolved
specs/model-avro.spec/src/main/scripts/io/aklivity/zilla/specs/model/avro/config/event.yaml
Outdated
Show resolved
Hide resolved
218b074
to
935a84e
Compare
720d608
to
016ca37
Compare
…untime/catalog/filesystem/internal/FilesystemEventFormatter.java Co-authored-by: John Fallows <john@aklivity.io>
…e/exporter/stdout/internal/StdoutExporterContext.java Co-authored-by: John Fallows <john@aklivity.io>
…exporter/otlp/internal/serializer/EventReader.java Co-authored-by: John Fallows <john@aklivity.io>
405c718
to
1ba8a8e
Compare
7a3884b
to
4086ca4
Compare
followup changes needed that will require more extensive changes:
|
specs/binding-mqtt-kafka.spec/src/main/resources/META-INF/zilla/mqtt_kafka.idl
Show resolved
Hide resolved
@@ -23,7 +23,8 @@ telemetry: | |||
events: | |||
- qname: test.net0 | |||
id: guard.jwt.authorization.failed | |||
message: AUTHORIZATION_FAILED user | |||
name: GUARD_JWT_AUTHORIZATION_FAILED | |||
message: No active session found for token identity (user). |
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.
This message seems to imply we are looking for matching state in zilla and couldn't find it.
However, I think this event is logged when an attempt to authorize the user (by validating the token) fails, with no implication of state expected to be found at zilla.
Let's reword to capture the underlying behavior without implying expectation to find server-side state, agree?
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.
Updated, please review the new wording with the added reason
field.
@@ -23,7 +23,8 @@ telemetry: | |||
events: | |||
- qname: test.net0 | |||
id: model.avro.validation.failed | |||
message: VALIDATION_FAILED java.io.EOFException | |||
name: MODEL_AVRO_VALIDATION_FAILED | |||
message: A message payload failed validation. java.io.EOFException. |
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.
Please remove exception class name from message.
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.
"java.io.EOFException"
is one of the exception messages returned from the AvroRuntimeException
class. The error
string passed in the event is set from various exception messages.
I think this is just a bad example of what kind of errors might come through and I don't know how the tests makes this specific exception happen or how to change it to something else.
origional comment here: #1029 (comment)
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 also added fixing this as a note in some the follow on modifications that are needed: #1029 (comment)
.../guard-jwt/src/main/java/io/aklivity/zilla/runtime/guard/jwt/internal/JwtEventFormatter.java
Outdated
Show resolved
Hide resolved
...rc/main/java/io/aklivity/zilla/runtime/catalog/karapace/internal/KarapaceEventFormatter.java
Outdated
Show resolved
Hide resolved
...rc/main/java/io/aklivity/zilla/runtime/catalog/karapace/internal/KarapaceEventFormatter.java
Outdated
Show resolved
Hide resolved
...ding-tls/src/main/java/io/aklivity/zilla/runtime/binding/tls/internal/TlsEventFormatter.java
Outdated
Show resolved
Hide resolved
…nding/tls/internal/TlsEventFormatter.java Co-authored-by: John Fallows <john@aklivity.io>
1216049
to
6377a8a
Compare
Description
This will use the defined internal
eventId
to format an observability-friendly in an all-caps snake case.Fixes #1013
docs PR: