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

[pkg/ottl] [Time] Add support for format and timezone #32140

Closed
4 tasks done
michalpristas opened this issue Apr 3, 2024 · 12 comments
Closed
4 tasks done

[pkg/ottl] [Time] Add support for format and timezone #32140

michalpristas opened this issue Apr 3, 2024 · 12 comments
Labels
enhancement New feature or request pkg/ottl priority:p2 Medium

Comments

@michalpristas
Copy link
Contributor

michalpristas commented Apr 3, 2024

Component(s)

pkg/ottl

Is your feature request related to a problem? Please describe.

atm Time is simple and converts string to time.Time

Describe the solution you'd like

supporting custom format, timezone seems like a must for some of my use cases
and locale would be really nice to have

Describe alternatives you've considered

No response

Additional context

No response

Tasks

@michalpristas michalpristas added enhancement New feature or request needs triage New item requiring triage labels Apr 3, 2024
Copy link
Contributor

github-actions bot commented Apr 3, 2024

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@TylerHelmuth
Copy link
Member

@michalpristas it seems that our docs for the Time function need some help.

The Time function requires as the first parameter the string to be converted and in the second parameter the format of the time the string represents.

We also should include in the Time function's section these conventions, as they are what let you specify the format of the time string, including timezone.

I agree that we could add support for locale. I believe this would be done via an optional string parameter.

Would you like to work on this?

@TylerHelmuth TylerHelmuth added priority:p2 Medium and removed needs triage New item requiring triage labels Apr 3, 2024
@michalpristas
Copy link
Contributor Author

yes, i believe this can be split in 2 PRs, one for doc one for locale

@michalpristas
Copy link
Contributor Author

michalpristas commented Apr 4, 2024

@TylerHelmuth as go does not support localization of datetime by default, i'm considering pulling some 3rd party library.
something like https://github.com/goodsign/monday

do you prefer internal implementation or something third party. I'm fine with both, implementation is not that difficult and we only need parsing part of that

@TylerHelmuth
Copy link
Member

@michalpristas The library we are using now supports it. It's the same library stanza uses.

@michalpristas
Copy link
Contributor Author

michalpristas commented Apr 4, 2024

you sure it supports locales? what i'm looking at is internal/coreinternal/timeutils/parser.go
as a library it supports specifying custom location which i want to leverage when handling timezone arg

but i'm not finding support for locales such as it-IT when the tool i'm observing is producing Italian logs in a form of e.g mercoledì set 4 2024 parsing with a layout Monday Jan 2 2006 would yield 4th September 2024

maybe i'm just looking at the wrong place. i also went through go.mod dependencies but found nothing really

@TylerHelmuth
Copy link
Member

You're right, I was thinking of the timeutils.GetLocation call handling timezone.

I agree we'd need to add a library or update timeutils to support this. @djaglowski how does stanza handle this?

@djaglowski
Copy link
Member

pkg/stanza has a location parameter. This is already part of timeutils so this may just be a matter of exposing it through the OTTL function?

@djaglowski
Copy link
Member

Ahh, nevermind. I was looking at time zone considerations only. I agree if we want to translate between languages we'll need to add new functionality.

@michalpristas
Copy link
Contributor Author

thank you for looking into it, will work on adding functionality into our existing library as part of this issue

@michalpristas
Copy link
Contributor Author

michalpristas commented Apr 5, 2024

my plan here is 4 PRs to keep them small:

  • updated docs [pkg/ottl] Updated docs for Time converter  #32185
  • handling timezone leveraging location support we already have in a library(would location be better name than timezone i think i'm more in favor of location now)
  • adding support for locale into our time library
  • exposing locale in Time converter

will make sure docs update is part of every change

TylerHelmuth added a commit that referenced this issue Apr 17, 2024
**Description:** 
Updated docs for ottl converter so it includes more information about
substitutes users can use

Few examples with timezone handling were added

**Link to tracking Issue:** #32140 

**Testing:** no testing

**Documentation:**

---------

Co-authored-by: Curtis Robert <crobert@splunk.com>
Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
rimitchell pushed a commit to rimitchell/opentelemetry-collector-contrib that referenced this issue May 8, 2024
**Description:** 
Updated docs for ottl converter so it includes more information about
substitutes users can use

Few examples with timezone handling were added

**Link to tracking Issue:** open-telemetry#32140 

**Testing:** no testing

**Documentation:**

---------

Co-authored-by: Curtis Robert <crobert@splunk.com>
Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
TylerHelmuth added a commit that referenced this issue May 9, 2024
**Description:** 
Added support for default timezone in Time converter. 
Timezone is optional and can be specified as so: `Time("2023-05-26
12:34:56", "%Y-%m-%d %H:%M:%S", "America/New_York")`

**Link to tracking Issue:** #32140

**Testing:** Unit tests added

**Documentation:** Documentation in ottl/Readme updated

---------

Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
@michalpristas
Copy link
Contributor Author

I extracted work on locales into 2 separate issues #32977 and #32978
should make it easier for people to pick them up, we can close this one as it was addressed in #32479

jlg-io pushed a commit to jlg-io/opentelemetry-collector-contrib that referenced this issue May 14, 2024
…ry#32479)

**Description:** 
Added support for default timezone in Time converter. 
Timezone is optional and can be specified as so: `Time("2023-05-26
12:34:56", "%Y-%m-%d %H:%M:%S", "America/New_York")`

**Link to tracking Issue:** open-telemetry#32140

**Testing:** Unit tests added

**Documentation:** Documentation in ottl/Readme updated

---------

Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request pkg/ottl priority:p2 Medium
Projects
None yet
Development

No branches or pull requests

3 participants