Skip to content

lua: timestamp in millis#14522

Merged
mattklein123 merged 16 commits intoenvoyproxy:mainfrom
mindyor:lua-timestamp-millis
Mar 28, 2021
Merged

lua: timestamp in millis#14522
mattklein123 merged 16 commits intoenvoyproxy:mainfrom
mindyor:lua-timestamp-millis

Conversation

@mindyor
Copy link
Copy Markdown
Contributor

@mindyor mindyor commented Dec 28, 2020

Commit Message: add Lua function to get timestamp in milliseconds and nanoseconds
Additional Description: Description: adds Lua function to get timestamp in higher resolution. Resolutions are: milliseconds since epoch. Resolution defaults to milliseconds since epoch.
Risk Level: low
Testing: added unit tests for default case, resolution levels, and invalid input
Docs Changes: added documentation for function in lua_filter.rst
Release Notes: added release note in version_history.rst
Platform Specific Features: n/a
[Optional Runtime guard:]
[Optional Fixes #Issue] #10282
[Optional Deprecated:]

((an update of #10329))

@repokitteh-read-only
Copy link
Copy Markdown

Hi @mindyor, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #14522 was opened by mindyor.

see: more, trace.

@dio
Copy link
Copy Markdown
Member

dio commented Dec 28, 2020

Thanks, @mindyor! Lua API looks good. Could you fix CI?

@dio dio self-assigned this Dec 28, 2020
Base automatically changed from master to main January 15, 2021 23:02
@repokitteh-read-only
Copy link
Copy Markdown

🙀 Error while processing event:

evaluation error
error: context deadline exceeded
🐱

Caused by: #14522 was edited by mattklein123.

see: more, trace.

@dio
Copy link
Copy Markdown
Member

dio commented Feb 3, 2021

@mindyor, do you want me to help you to finish this PR? :).

@dio
Copy link
Copy Markdown
Member

dio commented Feb 3, 2021

@mindyor when you have time, please have a look at https://github.com/envoyproxy/envoy/compare/main...dio:lua-timestamp-millis-updated?expand=1. I added two commits (one of them is to "rebase" it with the current main branch "main", the second one is to make it compiles). I think we need to add tests for running registerGlobal with some initializers (as use-case tests) in test/extensions/filters/common/lua/lua_test.cc. Hope this helps! Thanks!

Copy link
Copy Markdown
Member

@dio dio left a comment

Choose a reason for hiding this comment

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

Please add tests when calling registerGlobal (most likely you need to add it in test/extensions/filters/common/lua/lua_test.cc).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry, my bad, seems like we need to copy the initializers vector.

tls_slot_->runOnAllThreads([global, initializers](OptRef<LuaThreadLocal> tls) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Changed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you comment why this needs to be exactly 1? :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm it may not be. Taking this out

@dio
Copy link
Copy Markdown
Member

dio commented Feb 5, 2021

This cd870da (in #14882) I think will remedy clang-tidy's complaints.

@dio
Copy link
Copy Markdown
Member

dio commented Feb 6, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14522 (comment) was created by @dio.

see: more, trace.

Copy link
Copy Markdown
Member

@dio dio left a comment

Choose a reason for hiding this comment

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

Looks good. A request to expand the docs a bit more. Thanks!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we need to document the exported enums here?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you expand this, so we are aware of the enums.

@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #14522 was synchronize by mindyor.

see: more, trace.

@mindyor mindyor force-pushed the lua-timestamp-millis branch 2 times, most recently from 127c801 to df5e616 Compare March 22, 2021 18:12
Signed-off-by: mindyor <mor@squareup.com>
Signed-off-by: mindyor <mor@squareup.com>
@mindyor mindyor force-pushed the lua-timestamp-millis branch from df5e616 to 8ba3f45 Compare March 22, 2021 18:25
@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Mar 22, 2021
@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #14522 was synchronize by mindyor.

see: more, trace.

@moderation
Copy link
Copy Markdown
Contributor

Looks like you need to merge main. Quite a few conflicts

Signed-off-by: mindyor <mor@squareup.com>
Signed-off-by: mindyor <mor@squareup.com>
Signed-off-by: mindyor <mor@squareup.com>
Signed-off-by: mindyor <mor@squareup.com>
Signed-off-by: mindyor <mor@squareup.com>
Signed-off-by: mindyor <mor@squareup.com>
Signed-off-by: mindyor <mor@squareup.com>
Signed-off-by: mindyor <mor@squareup.com>
Signed-off-by: mindyor <mor@squareup.com>
…y lua int limits. update documentation

Signed-off-by: mindyor <mor@squareup.com>
@mindyor mindyor force-pushed the lua-timestamp-millis branch from 8ba3f45 to 488bcb7 Compare March 23, 2021 07:00
Signed-off-by: mindyor <mor@squareup.com>
@htuch htuch removed the api label Mar 23, 2021
mindyor-sq and others added 3 commits March 23, 2021 10:13
Signed-off-by: mindyor <mor@squareup.com>
Signed-off-by: mindyor <mor@squareup.com>
@mindyor
Copy link
Copy Markdown
Contributor Author

mindyor commented Mar 26, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14522 (comment) was created by @mindyor.

see: more, trace.

Copy link
Copy Markdown
Member

@dio dio left a comment

Choose a reason for hiding this comment

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

Thanks, @mindyor! Looks good!

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks!

@mattklein123 mattklein123 merged commit 486cd88 into envoyproxy:main Mar 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deps Approval required for changes to Envoy's external dependencies

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants