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

Add processor to set timezone for an event #3902

Merged
merged 13 commits into from
Apr 7, 2017

Conversation

martinscholz83
Copy link
Contributor

This PR add a new processor to set timzone for an event. See #3867

@elasticmachine
Copy link
Collaborator

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

1 similar comment
@elasticmachine
Copy link
Collaborator

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.


func newAddLocale(c common.Config) (processors.Processor, error) {
config := struct {
TimeZone string `config:"timezone"`
Copy link
Member

@andrewkroh andrewkroh Apr 4, 2017

Choose a reason for hiding this comment

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

Should this just be completely automatic using time.Now().Location()? Example: https://play.golang.org/p/2Lpzo3KvWJ

Copy link
Contributor Author

@martinscholz83 martinscholz83 Apr 4, 2017

Choose a reason for hiding this comment

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

That's what i not understand correctly. I thought we give the user the option to set the timezone. If not so you are right.

Copy link
Member

@andrewkroh andrewkroh Apr 4, 2017

Choose a reason for hiding this comment

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

I think idea is to report the local timezone. It probably shouldn't be configurable at all. Just detect the local TZ and report it in the event.

Once you have the timezone, you can use Logstash to do further transformations on the data (like interpreting syslog dates that are missing the timezone) or converting @timestamp to the machine's local timezone (not recommended IMHO).

A more specific use case is analyzing logon events, the timezone can be used to determine if the event occurred during normal working hours. Then you can alert on people working after hours and send them on a vacation.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like there's an even more direct way: time.Local.String().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

time.Local.String() only gives you Local. I have changed the method to get the the timezone

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, am I misunderstanding the docs at https://golang.org/pkg/time/#Location ?

Local represents the system's local time zone.

var Local *Location = &localLoc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On golang Playground i get UTC. On my machine i get

"beat": {
    "hostname": "4201halwsd00001",
    "name": "4201halwsd00001",
    "timezone": "Local",
    "version": "6.0.0-alpha1"
  },

???

Copy link
Member

Choose a reason for hiding this comment

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

Yep, you are right. I get the same thing "Local". So zone, _ := time.Now().Zone() should be sufficient because Now() always returns local time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Changed up to time.Now().Zone(). I was not sure if Now() always returns local time.

}

func (l addLocale) Run(event common.MapStr) (common.MapStr, error) {
zone, err := time.LoadLocation(l.timezone)
Copy link
Member

Choose a reason for hiding this comment

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

The Run method should be as simple and fast as possible. In newAddLocale() you I think can do this work once and store the result so that Run just becomes a map.Put() operation.

}

func newAddLocale(c common.Config) (processors.Processor, error) {
config := struct {
Copy link
Member

Choose a reason for hiding this comment

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

This whole config definition and the Unpack call can now be removed.

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

👍

@martinscholz83
Copy link
Contributor Author

Can we use this for the changelog?
Add new add_locale processor to export the local timezone with an event

@ruflin
Copy link
Member

ruflin commented Apr 4, 2017

@maddin2016 Changelog entry LGTM. Don't forget to add the PR number at the end (see other lines)

@andrewkroh
Copy link
Member

The config.full.yml files should mention the new processor. https://github.com/elastic/beats/blob/master/libbeat/_meta/config.full.yml#L45-L46

@ohadravid
Copy link
Contributor

Hi,
I see that the locale is only read once on the beat init (if I understand the code correctly).
However, we usually want to track long-running servers which might go between DST and regular time, and the change won't be reflected if we use the plugin as it as.

@martinscholz83
Copy link
Contributor Author

martinscholz83 commented Apr 5, 2017

Hi @ohadravid. For now we only export the timzone as a name CET, UTC, .... Maybe in the future we add an option to export the local time to a field, let say localtime. And then, you are right we have to move that logic into the Run method. @andrewkroh, it seems that there is a need for such an option to export the local time. What do you think to add this option now.

@martinscholz83
Copy link
Contributor Author

@ohadravid sorry 🙈 . You are right. Now we have CEST instead of CET.

@ruflin
Copy link
Member

ruflin commented Apr 5, 2017

Servers traveling in time ... 🙈 We should report the time zone without DST.

BTW this reminds me we will need to add docs to show people what the output of this will be and how to configure it.

@ohadravid
Copy link
Contributor

I think it will be more generic to just re-read the value at Run, since I do want to know the server is (or isn't) in DST (The fact that I know that CET should translate now to CEST does not mean the server really moved to the DST timezone).
It also affects computers which move between timezones (like laptops abroad).

@martinscholz83
Copy link
Contributor Author

Tested locally and timzone is exported as CEST. So i think that is fine, @ruflin?

@ohadravid
Copy link
Contributor

@maddin2016 consider these two cases:

  1. A server moves to DST.
    We install the beat and start it, it sends CET with each event.
    The server then moves to daylight savings times.
    The beat will still send events with CET (until the beat/server are restarted).

  2. A laptop which travels abroad.
    We install the beat, it sends CET with each event.
    The owner of the laptop fly to New York, change the timezone to EST.
    The beat will still send events with CET (until the beat/laptop are restarted).

To me it is very important we re-read the timezone value on every event, so we can provide reliable data about the event's time as much as we can (For example, we might want to search for logins at odd hours (middle of the night) on the laptop).

Also, it's really nice to be able to see and be a part of the development effort. Thank you for making this so transparent :)

@ruflin
Copy link
Member

ruflin commented Apr 5, 2017

@ohadravid Thanks for the inputs. Laptops moving from one time zone to an other is definitively something I was not thinking of. Just changed my opinion about the above then :-) @andrewkroh WDYT?

In case the processor is enabled, that means it is "refreshed" every time an even is created which can be up to 100k/s. In case we hit some issue here in the future we can think about caching it at least for 1s or something similar, but lets hit that wall first.

- add_locale:
-------------------------------------------------------------------------------

NOTE: Please consider that `add_locale` differentiate between DST and regular time.
Copy link
Member

Choose a reason for hiding this comment

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

s/differentiate/differentiates/

@andrewkroh
Copy link
Member

The beat.timestamp field needs to be added to the common fields.yml at https://github.com/elastic/beats/blob/master/libbeat/_meta/fields.common.yml#L8-L20.

@andrewkroh
Copy link
Member

With changes due to daylight savings and laptops moving around the world, making the timezone automatically update is a requirement. So let's do that. But I would like to understand the impact. @maddin2016 could you run two benchmarks, one as the code is now, and one where it reads the timezone on each Run() and post the results. If you don't have time I can do it at a little later.

@martinscholz83
Copy link
Contributor Author

Here are the results. The first value is as the code is now. And the second is where it reads the timezone on each run
image

func BenchmarkConstruct(b *testing.B) {
	var testConfig = common.NewConfig()

	input := common.MapStr{}

	p, err := newAddLocale(*testConfig)
	if err != nil {
		b.Fatal(err)
	}

	for i := 0; i < b.N; i++ {
		_, err = p.Run(input)
	}
}

@ruflin
Copy link
Member

ruflin commented Apr 6, 2017

@maddin2016 Thanks for the benchmarks. I would suggest to keep the benchmark code in the tests as it will be also useful to have these in the future.

BTW: Don't forget make update 😆

@martinscholz83
Copy link
Contributor Author

What about to move the logic into the run method?

@ruflin
Copy link
Member

ruflin commented Apr 6, 2017

@maddin2016 Which logic are you referring to?

@martinscholz83
Copy link
Contributor Author

Sorry. I meant the read for timezone.

}

func (l addLocale) Run(event common.MapStr) (common.MapStr, error) {
event.Put("beat.timezone", l.timezone)
Copy link
Member

Choose a reason for hiding this comment

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

Given the requirement to have this field automatically reflect the latest timezone, let's not cache the timezone like I requested previous (sorry for the change) and make Run always call time.Now().Zone(). Based on the benchmark results there wasn't a huge cost to doing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants