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 format option for add_locale processor #4106

Merged
merged 8 commits into from May 9, 2017

Conversation

Projects
None yet
5 participants
@martinscholz83
Copy link
Contributor

commented Apr 25, 2017

Like discussed in #3974, this PR add the option to format the timezoneoutput. I have decided to just use abbrevation and offset as options. Because is not so trivial to format time to a canonical_id. Here is a discussion about the topic. One thing which have to decide is should be format option required or should be abbrevation the default option.

@elasticmachine

This comment has been minimized.

Copy link

commented Apr 25, 2017

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

This comment has been minimized.

Copy link

commented Apr 25, 2017

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.

@andrewkroh
Copy link
Member

left a comment

Looks like you are off to a good start. Thanks for working on this!

}

func (t TimezoneFormat) String() string {
return timezoneFormats[t]

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Apr 26, 2017

Member

It seems like this could easily result in a panic with something like TimezoneFormat(-1).String(). I think a more common approach is to store a map[TimezoneFormat]string. But you can use an array like you did, but you just need to bounds checking on t like (t > 0 && t < len(timezoneFormats)).

return addLocale{}, nil
config := struct {
Format string `config:"format" validate:"required"`
}{}

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Apr 26, 2017

Member

Looks like this is missing the default value initialization.

This comment has been minimized.

Copy link
@martinscholz83

martinscholz83 Apr 27, 2017

Author Contributor

That's what i asked. How do you think should that processor work

-add_locale:

with default abbrevation
or

-add_locale:
  format: "abbrevation/offset"

make format as required.

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh May 4, 2017

Member

I think it should default to offset. IMO that would be easier to use and not require someone to have a lookup table of names to numbers and would account for DST changes.

}{}
err := c.Unpack(&config)
if err != nil {
return nil, fmt.Errorf("fail to unpack the include_fields configuration: %s", err)

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Apr 26, 2017

Member

s/include_fields/add_locale/

case "offset":
loc.TimezoneFormat = Offset
default:
return nil, errors.New("given format is not valid")

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Apr 26, 2017

Member

More details in the error would be better for users running -configcheck. Things like: What is the invalid value that was given? What is the thing that caused the error (add_locale)? How should the error be fixed (provide the two allowed values)?

}

func (l addLocale) Run(event common.MapStr) (common.MapStr, error) {
zone, _ := time.Now().Zone()
event.Put("beat.timezone", zone)
format := l.Format()

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Apr 26, 2017

Member

How do the old benchmarks compare to the new benchmarks now that Format() is called for each event? It would be good to test with both Abbreviation and Offset.

case Abbrevation:
fmt, _ = tm.Zone()
case Offset:
//loc, _ := time.LoadLocation("America/Belize")

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Apr 26, 2017

Member

This should be removed.

This comment has been minimized.

Copy link
@martinscholz83

martinscholz83 Apr 27, 2017

Author Contributor

tests 😄

}

fmt += strconv.Itoa(offset)
fmt += ":00"

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Apr 26, 2017

Member

Timezone offsets do not all fall on one hour boundaries. See https://en.wikipedia.org/wiki/List_of_tz_database_time_zones

Here's an example calculation: https://play.golang.org/p/iuiuSxMZ1Q

This will need some test cases.

This comment has been minimized.

Copy link
@martinscholz83

martinscholz83 Apr 27, 2017

Author Contributor

What is the correct form for positive and negative values to display.
11:45, 02:45 and -11:45, -01:45 or 11:45, 2:45 and -11:45, -1:45. Its about the leading zero and the positive and negative sign.

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh May 4, 2017

Member

From the wiki page I linked above:

The UTC offsets (columns 5 and 6) are parsed from the "continent files" and accordingly ISO 6709 are in the format {+|-}hh:mm

So I'd say go with {+|-}hh:mm.

@andrewkroh

This comment has been minimized.

Copy link
Member

commented Apr 26, 2017

jenkins, test it

martinscholz83 added some commits Apr 27, 2017

@ruflin

This comment has been minimized.

Copy link
Collaborator

commented May 4, 2017

@maddin2016 One of the tests is failing: add_locale_test.go:38: fail to unpack the include_fields configuration: missing required field accessing 'format'

@martinscholz83

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2017

Hi @ruflin, that's right. There 2 questions which are unanswered. Then i can continue.

How do you think this processor should work

-add_locale:

with default abbrevation
or

-add_locale:
  format: "abbrevation/offset"

make format as required.

What is the correct form for positive and negative values to display.
11:45, 02:45 and -11:45, -01:45 or 11:45, 2:45 and -11:45, -1:45. Its about the leading zero and the positive and negative sign.

martinscholz83 added some commits May 5, 2017

@ruflin

This comment has been minimized.

Copy link
Collaborator

commented May 9, 2017

@monicasarbu @andrewkroh Can one of you comment on the above?

@martinscholz83

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2017

Hi @ruflin. @andrewkroh has answered my question. Can you start the integration tests again. Something went wrong with travis. Thanks.

@ruflin

This comment has been minimized.

Copy link
Collaborator

commented May 9, 2017

Nice, just restarted it.

@ruflin

This comment has been minimized.

Copy link
Collaborator

commented May 9, 2017

jenkins, test it

TimezoneFormat TimezoneFormat
}

type TimezoneFormat int

This comment has been minimized.

Copy link
@elasticmachine

elasticmachine May 9, 2017

[golint] reported by reviewdog 🐶
exported type TimezoneFormat should have comment or be unexported

type TimezoneFormat int

const (
Abbrevation TimezoneFormat = iota

This comment has been minimized.

Copy link
@elasticmachine

elasticmachine May 9, 2017

[golint] reported by reviewdog 🐶
exported const Abbrevation should have comment (or a comment on this block) or be unexported

martinscholz83 added some commits May 9, 2017

@andrewkroh andrewkroh merged commit 79ebc5c into elastic:master May 9, 2017

4 checks passed

codecov/patch 75% of diff hit (target 64.61%)
Details
codecov/project Absolute coverage decreased by -1.56% but relative coverage increased by +10.38% compared to af60958
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dedemorton

This comment has been minimized.

Copy link
Contributor

commented May 16, 2017

@andrewkroh Shouldn't there be a doc change for this, too?

@andrewkroh

This comment has been minimized.

Copy link
Member

commented May 16, 2017

I updated the docs in #4330

ramon-garcia added a commit to ramon-garcia/beats that referenced this pull request Dec 5, 2017

Add format option for add_locale processor (elastic#4106)
* Add format option for timezone
* Make offset default format option
* Add descriptions

athom added a commit to athom/beats that referenced this pull request Jan 25, 2018

Add format option for add_locale processor (elastic#4106)
* Add format option for timezone
* Make offset default format option
* Add descriptions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.