Skip to content
This repository was archived by the owner on Mar 11, 2026. It is now read-only.

fix: Correction for timestamp and instrumentation record severity fields format#1328

Merged
losalex merged 3 commits intomainfrom
losalex/fix-cleanup
Sep 3, 2022
Merged

fix: Correction for timestamp and instrumentation record severity fields format#1328
losalex merged 3 commits intomainfrom
losalex/fix-cleanup

Conversation

@losalex
Copy link
Copy Markdown
Contributor

@losalex losalex commented Sep 2, 2022

It seems that timestamp format for log records printed to stdout are in wrong format - the right format is defined here: timestamp-processing. Worth mentioning that changing this format should be safe despite a fact that StructuredJson defined as external - it is used internally and I believe it is safe to change format of timestamp.
Also instrumentation record was setting severity as number and since it should be with DEFAULT severity, removing it from instrumentation record.

Related to # 1266 🦕

@losalex losalex requested review from a team September 2, 2022 23:36
@product-auto-label product-auto-label Bot added size: m Pull request size is medium. api: logging Issues related to the googleapis/nodejs-logging API. labels Sep 2, 2022
Copy link
Copy Markdown
Contributor

@daniel-sanche daniel-sanche left a comment

Choose a reason for hiding this comment

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

Left a couple comments/sugg

Comment thread src/entry.ts Outdated
Comment thread src/utils/common.ts Outdated
Comment thread test/entry.ts Outdated
Comment thread test/entry.ts Outdated
Comment thread test/entry.ts
Comment thread src/entry.ts
Copy link
Copy Markdown
Contributor

@daniel-sanche daniel-sanche left a comment

Choose a reason for hiding this comment

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

I'd suggest considering using RFC3339 instead of this format, and possibly expanding the tests, but overall LGTM as is

Opened 1329 for this suggestion. I believe the only benefit of this is readability for raw JSON payload printed to stdout and/or HTTP/S, hence making this P3.

@losalex losalex added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 3, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 3, 2022
@losalex losalex merged commit 057431b into main Sep 3, 2022
@losalex losalex deleted the losalex/fix-cleanup branch September 3, 2022 01:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

api: logging Issues related to the googleapis/nodejs-logging API. size: m Pull request size is medium.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants