Skip to content
This repository was archived by the owner on Oct 3, 2023. It is now read-only.

Metrics: use number for timestamp instead of interface#196

Closed
mayurkale22 wants to merge 1 commit intocensus-instrumentation:masterfrom
mayurkale22:Metric_Timestamp
Closed

Metrics: use number for timestamp instead of interface#196
mayurkale22 wants to merge 1 commit intocensus-instrumentation:masterfrom
mayurkale22:Metric_Timestamp

Conversation

@mayurkale22
Copy link
Copy Markdown
Member

No description provided.

Copy link
Copy Markdown
Contributor

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

And what is the unit?

@mayurkale22
Copy link
Copy Markdown
Member Author

Are you asking about unit tests or unit type in MetricDescriptor?

@bogdandrutu
Copy link
Copy Markdown
Contributor

bogdandrutu commented Nov 20, 2018

Timestamp as a number should have a unit "us" or "ns" from the epoch. I am a bit lost in this PR logic.

@mayurkale22
Copy link
Copy Markdown
Member Author

I was thinking to use Date.now() (to represent a time at which the value/gauge is recorded - format: milliseconds since epoch) for TimeSeries, Point and Exemplar timestamp attribute.
Looks like we did same thing here: https://github.com/census-instrumentation/opencensus-node/blob/master/packages/opencensus-core/src/stats/types.ts#L136

@bogdandrutu
Copy link
Copy Markdown
Contributor

https://www.geeksforgeeks.org/javascript-date-now/ so it seems that the timestamp you are proposing is millis since epoch. Do we have a way to get better timestamps (ns or us granularity)?

@isaikevych isaikevych self-requested a review November 28, 2018 00:36
@isaikevych
Copy link
Copy Markdown
Contributor

https://www.geeksforgeeks.org/javascript-date-now/ so it seems that the timestamp you are proposing is millis since epoch. Do we have a way to get better timestamps (ns or us granularity)?

@mayurkale22 I dont know what are you going to achieve by this changes but anyway
@bogdandrutu nodejs has process.hrtime() which returns high-resolution real time in [seconds, nanoseconds] array then we had plans to use it and that is why protobuf generated Timestamp interface with seconds and nanos

@mayurkale22
Copy link
Copy Markdown
Member Author

As discussed, will use process.hrtime() to get timstamp in ns or us granularity. Closing this PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants