fix(gcp): allow logging message as jsonPayload

[felangel]

Closes #394

Currently, log messages do not get interpreted as jsonPayload even if a JSON object is provided:

log({'hello': 'world'});

results in:

textPayload: "{hello: world}"

This change results in the following log:

jsonPayload: {
  message: {
    hello: world
  }
}

Are there existing tests for the logger? I looked but wasn't able to find any.

[kevmoo]

gah! this is tricky.

Before this change EVERY message is encoded with toString with this change all messages are encoded with jsonEncode. Which might cause failures in a bunch of unexpected cases.

I wonder if we should add a JsonMessage wrapper type that we can test against? See where I'm going?

[felangel]

gah! this is tricky.

Before this change EVERY message is encoded with toString with this change all messages are encoded with jsonEncode. Which might cause failures in a bunch of unexpected cases.

I wonder if we should add a JsonMessage wrapper type that we can test against? See where I'm going?

Just to make sure I understand, you're suggesting introducing a JsonMessage class that developers will need to explicitly wrap their messages in like:

log(JsonMessage({'hello': 'world'}));

Is that correct?

[kevmoo]

@felangel – suggesting/brainstorming/etc – yeah. Thoughts?

[felangel]

@kevmoo what are your thoughts on checking if the message is json encodable and defaulting to string if not? Would that be too expensive of an operation?

[kevmoo]

@kevmoo what are your thoughts on checking if the message is json encodable and defaulting to string if not? Would that be too expensive of an operation?

I guess we could just test is Map. Should be fine. A tiny bit hacky, but I doubt it'd break anyone.

[felangel]

@kevmoo what are your thoughts on checking if the message is json encodable and defaulting to string if not? Would that be too expensive of an operation?

I guess we could just test is Map. Should be fine. A tiny bit hacky, but I doubt it'd break anyone.

@kevmoo updated the PR lmk what you think?

[kevmoo]

@felangel – now the nits.

• [ ] test
• [ ] changelog entry
• [ ] Likely want to include some doc comments (maybe on cloudLoggingMiddleware) explaining this behavior?

[felangel]

@felangel – now the nits.

• [ ] test
• [ ] changelog entry
• [ ] Likely want to include some doc comments (maybe on cloudLoggingMiddleware) explaining this behavior?

Sounds great, I didn't see any tests for log but happy to try to add some from scratch.

[kevmoo]

@felangel – crap. If there is no testing, we should leave it for now. Plumbing through the right logic there is more work than you should have to do...

[felangel]

@felangel – crap. If there is no testing, we should leave it for now. Plumbing through the right logic there is more work than you should have to do...

@kevmoo sounds good, I updated api docs and CHANGELOG lmk if there's anything else you want me to do.

[kevmoo]

You're going to have to rebase, now. 😁

[felangel]

@kevmoo, just rebased on main 👍