-
Notifications
You must be signed in to change notification settings - Fork 71
feat: add native logging with headers and body to CloudEvent #437
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
Conversation
Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>
Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>
Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>
Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>
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.
Thanks for making this change, @grant - just a small request re: the test
src/event/cloudevent.ts
Outdated
* The native `console.log` value of the CloudEvent. | ||
* @return The string representation of the CloudEvent. | ||
*/ | ||
[Symbol.for("nodejs.util.inspect.custom")]() { |
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 is a pretty clever approach. :)
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.
Thanks! Came from: https://nodejs.org/api/util.html#util_util_inspect_custom
Co-authored-by: Lance Ball <lball@redhat.com>
I'm getting errors with the index type of the symbol. Want to go with the existing test and perhaps you can add changes if appropriate after? |
Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>
9b48e86
to
7da6533
Compare
I was able to get the test to pass like this: it("serializes as JSON with raw log", () => {
const ce = new CloudEvent({ ...fixture, data: { lunch: "tacos" } });
const sym = (Symbol.for("nodejs.util.inspect.custom") as unknown) as string;
const f = (ce[sym] as CallableFunction).bind(ce);
expect(ce.toString()).to.deep.equal(f());
}); Also, to eliminate the warnings that are being generated, the CE function should look like this (note /**
* The native `console.log` value of the CloudEvent.
* @return {string} The string representation of the CloudEvent.
*/
[Symbol.for("nodejs.util.inspect.custom")](): string {
return this.toString();
} |
Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>
ac667e0
to
f5965bc
Compare
Great, thanks @lance. PTAL. |
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.
LGTM
Add better native logging to the
CloudEvent
class.Proposed Changes
Logging a
CloudEvent
should call thetoString
of theCloudEvent
.Fixes #432