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

feat: add timestamp_to_datetime_format_expr optional rule paramameter #978

Conversation

thpiron
Copy link

@thpiron thpiron commented Oct 12, 2022

Description

No breaking changes.
The goal of this PR is to provide a way to transform a timestamp returned by Elasticsearch before parsing it into a datetime.
This come from an issue we have when using a custom timestamp field that can contains nanoseconds. For now, the parsing of the returned timestamp raise a Value error, as there is 3 additional digit that can't be parse in python (datetime.datetime.strptime format doesn't support nanoseconds). And I found no workaround (other that changing the timestamp format in Elasticsearch of course).
It seems logic that as we can choose a way to modify a timestamp before sending it to Elastisearch, we could also do the same when retrieving one.

So this PR simply add a rule parameter called timestamp_to_datetime_format_expr that does the same thing as timestamp_format_expr in the _ts_to_dt_with_format function instead of _dt_to_ts_with_format.
I didn't want to use the same rule parameter to avoid any breaking changes, and corner case I could not anticipate.

Checklist

  • I have reviewed the contributing guidelines.
  • I have included unit tests for my changes or additions.
  • I have successfully run make test-docker with my changes.
  • I have manually tested all relevant modes of the change in this PR.
  • I have updated the documentation.
  • I have updated the changelog.

Questions or Comments

Got some docker layer cache issues running make test-docker, will create another small PR to fix that.

@thpiron thpiron force-pushed the feat/add_pre_timestamp_to_datetime_format_expr_in_rules_parameters branch from eec14cc to 3230c11 Compare October 12, 2022 09:39
@jertel
Copy link
Owner

jertel commented Oct 13, 2022

Looks like a useful change. Would be nice to get the unit tests included with this PR though. What specific error are you seeing when running make test-docker?

@thpiron
Copy link
Author

thpiron commented Oct 13, 2022

I don't have any error when running make test-docker, every test passes. But that's not surprising as no test case exists for custom timestamp yet. At least I didn't find any.

No really sure where and how I should create those tests.
If you think it's worth it maybe I could create some in tests/base_test.py by taking as example tests like test_some_hits_unix?

@jertel
Copy link
Owner

jertel commented Oct 13, 2022

I suggest adding a new test to loaders_test.py and perhaps use test_kibana_discover_to_timedelta as a template.

@thpiron
Copy link
Author

thpiron commented Oct 13, 2022

Yes it's a good example thanks! I'll add those tests asap.

@thpiron
Copy link
Author

thpiron commented Oct 13, 2022

Added tests for timestamp_type custom, timestamp_to_datetime_format_expr and timestamp_format_expr.

@jertel
Copy link
Owner

jertel commented Oct 13, 2022

Looks good! Thanks for back-filling those related tests.

@jertel jertel merged commit c614962 into jertel:master Oct 13, 2022
@thpiron thpiron deleted the feat/add_pre_timestamp_to_datetime_format_expr_in_rules_parameters branch October 14, 2022 09:09
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 18, 2024
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.

2 participants