Skip to content

Conversation

@Hweinstock
Copy link
Contributor

Problem

Deeply nested objects fail to show up in the logger due to use of %O instead of JSON.stringify. Some customers use this information for debugging or other purposes.
Ex.

2024-11-12 13:23:39.682 [info] request from tab: tab-1 conversationID: 619df9a2-3ab2-4676-9896-9eb608d80582 request: {
  conversationState: {
    currentMessage: { userInputMessage: [Object] },
    chatTriggerType: 'MANUAL',
    customizationArn: undefined
  }
}

Solution

  • Change codewhisper chat request/response log messages (which are very deeply nested) back to JSON.stringify.

License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@Hweinstock Hweinstock requested a review from a team as a code owner November 12, 2024 19:11
@github-actions
Copy link

  • This pull request modifies code in src/* but no tests were added/updated.
    • Confirm whether tests should be added or ensure the PR description explains why tests are not required.

@Hweinstock Hweinstock requested a review from a team as a code owner November 13, 2024 14:37
showHidden: opt?.showHidden ?? false,
color: opt?.color ?? false,
}
const objToShow = partialClone(obj, options.depth, options.omitKeys, options.replacement)
Copy link
Contributor

@justinmk3 justinmk3 Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we skip the partialClone for the non-web case?

I think inspect() probably doesn't need to support partiaClone features (such as omitKeys) Because it's trivial for any caller to use inspect(partialClone(...)).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was worried that some of the options would then only apply in certain cases. Ex. if not on web and we pass omitKeys, it does nothing w/o error or warning. But I guess the caller can just wrap it themselves.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ex. if not on web and we pass omitKeys,

inspect() doesn't really need to support omitKeys though. Unless that is a feature of nodejs inspect().

Comment on lines 335 to 336
omitKeys: string[]
replacement: any
Copy link
Contributor

@justinmk3 justinmk3 Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inspect doesn't need this options, because callers can easily use inspect(partialClone(o)) to opt-in if they want (and that's a good general practice: composability instead of "denormalization").

The use of partialClone for the "web" case in our inspect wrapper, is simply a way for us to support depth.

Copy link
Contributor

@justinmk3 justinmk3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be mentioned in the lint rule?

@justinmk3 justinmk3 merged commit b753e82 into aws:master Nov 13, 2024
24 of 25 checks passed
@Hweinstock Hweinstock deleted the loggingNestedObj branch November 21, 2024 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants