Skip to content
This repository has been archived by the owner on Nov 8, 2022. It is now read-only.

Fixes #1118 - Getting started guide fails? #1122

Merged
merged 1 commit into from
Aug 12, 2016

Conversation

jcooklin
Copy link
Collaborator

@jcooklin jcooklin commented Aug 3, 2016

Fixes #1118

Sets the current time as the advertised time in the returned metric when using grpc.

Summary of changes:

  • Ensures that the advertised time of the metric is set when calling GetMetricTypes (from grpc client)
  • Checks that the advertised time is set when converting a grpc/common/metric to a core.Metric

Testing done:

  • Manual
  • Unit

@intelsdi-x/snap-maintainers

Sets the current time as the advertised time in the returned metric when using grpc.
var lastAdvertisedTime time.Time
// if the lastAdvertisedTime is not set we handle. -62135596800 represents the
// number of seconds from 0001-1970 and is the default value for time.Unix.
if mt.LastAdvertisedTime.Sec == int64(-62135596800) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is quite unexpected to have time replaced by Now() during conversion.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is due to timezone switch to LMT (not sure the exact cut off dates). Maybe another option is to only log in UTC?

$ go_time
t: 1900-01-01 00:00:00 +0000 UTC t in Local: 1899-12-31 16:07:02 -0752 LMT
t2: 2000-01-01 00:00:00 +0000 UTC t2 in Local: 1999-12-31 16:00:00 -0800 PST

Copy link
Contributor

@lmroz lmroz Aug 3, 2016

Choose a reason for hiding this comment

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

Wow, good catch @nanliu . I haven't been aware of that some places in the world (maybe Poland too, gotta check) had in the past different and fractional "time zone" ...and PCs actually take this into account 💯
We have some today, so I assumed author of the issue just lives in unlucky place that crashes Go ✌️ Fortunately they are fractional only in terms of hours and this function is crashing on fractional minutes.

@lmroz
Copy link
Contributor

lmroz commented Aug 3, 2016

I guess that this issue is caused by fractional timezone set when unmarshalling data from grpc (by calling time.Unix()), not getting empty data from there. So during conversion to time.Time it should be moved to UTC time zone (or just before marshalling to gob). Perhaps this PR is not going to fix the issue because I doubt that behavior of Now() will be different than Unix(), but I'm not betting money on that.

@pittma
Copy link
Contributor

pittma commented Aug 12, 2016

LGTM

@pittma pittma merged commit 30aa88d into intelsdi-x:master Aug 12, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants