Skip to content
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

fix: Fix timestamp drift due to using instanceof typeguard #827

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cindy-peng
Copy link
Contributor

@cindy-peng cindy-peng commented Nov 6, 2023

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #802 🦕
This is to fix timestamp drift issue which may cause timestamp inaccuracy while using OpenTelemetry and view trace timespans. Since timestamp within metadata is defined as a union-types including null and undefined, the type guard is not necessary here.

@cindy-peng cindy-peng requested review from a team as code owners November 6, 2023 21:13
@product-auto-label product-auto-label bot added size: xs Pull request size is extra small. api: logging Issues related to the googleapis/nodejs-logging-winston API. labels Nov 6, 2023
@@ -253,7 +253,7 @@ export class LoggingCommon {
// metadata. As Winston 3 buffers logs when a transport (such as this one)
// invokes its log callback asynchronously, a timestamp assigned at log time
// is more accurate than one assigned in a transport.
if (metadata.timestamp instanceof Date) {
Copy link
Contributor Author

@cindy-peng cindy-peng Nov 13, 2023

Choose a reason for hiding this comment

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

This typeguard is preventing customer metadata timestamp to be properly assigned to entry metadata. Considering entryMetadata is expecting a union type Timestamp | null | undefined, I think it is not necessary for us to force a Date type to be checked and assigned.

@daniel-sanche
Copy link
Contributor

Can you add any tests around this? Try to make one that would fail before the change, but passes after

@cindy-peng
Copy link
Contributor Author

Can you add any tests around this? Try to make one that would fail before the change, but passes after

Good suggestion. Will do!

@product-auto-label product-auto-label bot added the stale: old Pull request is old and needs attention. label Dec 7, 2023
@product-auto-label product-auto-label bot added stale: extraold Pull request is critically old and needs prioritization. and removed stale: old Pull request is old and needs attention. labels Jan 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: logging Issues related to the googleapis/nodejs-logging-winston API. size: xs Pull request size is extra small. stale: extraold Pull request is critically old and needs prioritization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OpenTelemetry issues; timestamp drift, and LOGGING_SAMPLED_KEY is not treated as boolean
2 participants