Add support for all CLP FFI functionality and refactor existing code: #3

davidlion · 2024-04-10T20:17:21Z

Add CLP encoding/decoding functionalities
Add CLP IR wildcard query support
Add basic GitHub templates and workflows
Rewrite CLP IR serialization/deserialization functionalities to match the latest CLP glossary, and simplify resource management
Add reader/writer interfaces for CLP IR
Update formatting and lining tooling
Switch to use static library

Description

all CLP encoding/decoding/searching FFI functionality
simplify resource management and control flow
- Go code will need to call Close() functions to avoid memory leaks, but code is much easier to understand and less error prone
switch to static library
rewrite/add documentation for all user-facing code
rewrite terminology to match new CLP glossary
update formatting + linting tooling
github templates + workflows

Validation performed

Old/existing unit tests remade and passing.
Added new unit tests.

Note, this is the same as #2 but from the beta branch to de-duplicate branches.

- Add all clp-ffi functions - Add reader and writer - Add unit tests - Add formatting and linting - Overhaul doc strings and terminology

… the header when used with a c++ compiler.

LinZhihao-723

Add the first batch of comments on cpp decoder/deserializer implementations

cpp/src/ffi_go/ir/LogTypes.hpp

cpp/src/ffi_go/LogTypes.hpp

cpp/src/ffi_go/ir/LogTypes.hpp

cpp/src/ffi_go/ir/deserializer.h

cpp/src/ffi_go/ir/deserializer.cpp

LinZhihao-723

Headers

cpp/src/ffi_go/ir/decoder.h

LinZhihao-723

Comments about the workflow

.github/workflows/build.yml

LinZhihao-723

Comments on the README

README.rst

LinZhihao-723

Add comments on cpp implementations of encoders and serializers

cpp/.clang-format

cpp/src/ffi_go/ir/decoder.cpp

cpp/src/ffi_go/ir/encoder.h

cpp/src/ffi_go/ir/serializer.h

cpp/src/ffi_go/ir/serializer.cpp

cpp/src/ffi_go/ir/serializer.h

LinZhihao-723

Comments on the wildcard query C++ implementations.

cpp/src/ffi_go/search/wildcard_query.h

cpp/src/ffi_go/search/wildcard_query.cpp

LinZhihao-723

Comments on ffi/ffi.go. All about the doc strings.

ffi/ffi.go

- fix typos - update NamespaceIndentation to None - add [[nodiscard]] - typedef -> using - add const - use fancy auto syntax Co-authored-by: Lin Zhihao <[email protected]>

- drop static cast Co-authored-by: Lin Zhihao <[email protected]>

Co-authored-by: Lin Zhihao <[email protected]>

.github/workflows/build.yml

cpp/src/ffi_go/ir/encoder.cpp

LinZhihao-723

According to the cpp standard, for dependent names we need typename disambiguator to explicitly tell the compiler this is a type name: https://en.cppreference.com/w/cpp/language/dependent_name
Even though the code still gets compiled in some compilers without typename, it is a good practice to add it anyways.

cpp/src/ffi_go/ir/decoder.cpp

cpp/src/ffi_go/ir/encoder.cpp

LinZhihao-723

Comments on ir/writer.go and ir/serializer.go

ir/serializer.go

ir/writer.go

LinZhihao-723

Comments on:

ir/ir.go
ir/decoder.go
ir/deserializer.go
search/wildcard_query.go

LinZhihao-723 · 2024-04-28T04:18:25Z

ir/ir.go

+// streams through CLP's FFI (foreign function interface). More details on CLP
+// IR streams are described in this [Uber blog].
+// Log events in IR format can be viewed in the [log viewer] or programmatically
+// analyzed using APIs provided in this package.
 //
 // [CLP]: https://github.com/y-scope/clp
 // [Uber blog]: https://www.uber.com/blog/reducing-logging-cost-by-two-orders-of-magnitude-using-clp/


Technically this line exceeds 100 chars. If wrapping it into a new line doesn't break the doc site we should do it

ir/ir.go

ir/deserializer.go

LinZhihao-723 · 2024-04-29T00:43:26Z

search/wildcard_query.go

+}
+
+// A timestamp interval of [m_lower, m_upper).
+type TimestampInterval struct {


Do we provide a way to create a default timestamp interval that covers the entire supported timestamp range?

This seems unnecessary as constructing the equivalent struct would also only be one line of code.

But do we have TS_MIN and TS_MAX defined?

Why would these be different from int min/max?

we defined the timestamp type to EpochTimeMs instead of explicitly using int. It might not be obvious for users what underlying type it is. It also introduces problems if we change the underlying type in the future.

search/wildcard_query.go

LinZhihao-723

Comments on ir/reader.go

ir/reader.go

Improves/rewrites/fixes many doc strings Co-authored-by: Lin Zhihao <[email protected]>

…max time interval.

LinZhihao-723

Need a multi-line commit message like this PR: y-scope/clp#301

davidlion · 2024-05-10T15:00:17Z

how about:

Add all CLP FFI functionality and refactor existing code: (#3)

Add CLP IR encoding/decoding
Add CLP IR wildcard query
Add basic GitHub templates and workflows
Rewrite CLP IR serialization/deserialization functionalities to match the latest CLP glossary, and simplify resource management
Add reader/writer interfaces for CLP IR
Provide pre-built static libraries

davidlion added 2 commits September 28, 2023 11:55

Large refactor and update

fba8f62

- Add all clp-ffi functions - Add reader and writer - Add unit tests - Add formatting and linting - Overhaul doc strings and terminology

Reintroduce __cplusplus ifdefs to headers so the correct names are in…

9253255

… the header when used with a c++ compiler.

davidlion requested review from kirkrodrigues and LinZhihao-723 April 10, 2024 20:17

LinZhihao-723 reviewed Apr 14, 2024

View reviewed changes

LinZhihao-723 requested changes Apr 24, 2024

View reviewed changes

cpp/src/ffi_go/ir/decoder.h Outdated Show resolved Hide resolved

LinZhihao-723 reviewed Apr 24, 2024

View reviewed changes

.github/workflows/build.yml Outdated Show resolved Hide resolved

LinZhihao-723 requested changes Apr 24, 2024

View reviewed changes

README.rst Outdated Show resolved Hide resolved

README.rst Show resolved Hide resolved

LinZhihao-723 requested changes Apr 24, 2024

View reviewed changes

cpp/src/ffi_go/search/wildcard_query.h Outdated Show resolved Hide resolved

cpp/src/ffi_go/search/wildcard_query.cpp Outdated Show resolved Hide resolved

cpp/src/ffi_go/search/wildcard_query.cpp Outdated Show resolved Hide resolved

LinZhihao-723 requested changes Apr 24, 2024

View reviewed changes

ffi/ffi.go Outdated Show resolved Hide resolved

ffi/ffi.go Outdated Show resolved Hide resolved

ffi/ffi.go Outdated Show resolved Hide resolved

davidlion and others added 7 commits April 25, 2024 12:53

Apply suggestions from code review

33647d0

- fix typos - update NamespaceIndentation to None - add [[nodiscard]] - typedef -> using - add const - use fancy auto syntax Co-authored-by: Lin Zhihao <[email protected]>

Apply suggestions from code review

2fd3f15

- drop static cast Co-authored-by: Lin Zhihao <[email protected]>

Improve writing

c4abd24

Fix extern C header ifdef to not wrap include statements.

50bec08

Add macos back in workflow

98176cb

LogTypes.hpp -> types.hpp

439f948

Update cpp/src/ffi_go/ir/decoder.cpp

a439133

Co-authored-by: Lin Zhihao <[email protected]>

LinZhihao-723 requested changes Apr 25, 2024

View reviewed changes

.github/workflows/build.yml Outdated Show resolved Hide resolved

cpp/src/ffi_go/ir/encoder.cpp Outdated Show resolved Hide resolved

Update cpp/src/ffi_go/ir/encoder.cpp

67003c2

LinZhihao-723 requested changes Apr 25, 2024

View reviewed changes

cpp/src/ffi_go/ir/decoder.cpp Outdated Show resolved Hide resolved

cpp/src/ffi_go/ir/encoder.cpp Outdated Show resolved Hide resolved

LinZhihao-723 and others added 3 commits April 25, 2024 16:23

Adding typename for dependent names for disambiguration purpose.

2d570a8

Refactor c++ and add missing null checks.

edeab0d

Rename LogEvent to LogEventStorage.

99e9cea

LinZhihao-723 requested changes Apr 27, 2024

View reviewed changes

ir/serializer.go Show resolved Hide resolved

ir/serializer.go Outdated Show resolved Hide resolved

ir/serializer.go Outdated Show resolved Hide resolved

ir/writer.go Show resolved Hide resolved

ir/writer.go Outdated Show resolved Hide resolved

LinZhihao-723 requested changes Apr 29, 2024

View reviewed changes

ir/reader.go Outdated Show resolved Hide resolved

ir/reader.go Outdated Show resolved Hide resolved

ir/reader.go Outdated Show resolved Hide resolved

ir/reader.go Show resolved Hide resolved

LinZhihao-723 and others added 3 commits April 28, 2024 21:41

Apply suggestions from code review

06ea187

Apply suggestions from code review

00b7904

Improves/rewrites/fixes many doc strings Co-authored-by: Lin Zhihao <[email protected]>

Refactor...

c6b9ec5

LinZhihao-723 and others added 11 commits April 29, 2024 17:10

Using api decoration.

8f7802c

Apply clang-tidy suggestions on src/ffi_go/ir/*

bc6fca9

Apply clang-tidy suggestions on regular files

3d06c36

Update github workflow to specify Go versions.

72e48bc

Workflow debugging.

89139e0

Add api_decoration header.

ef2eb59

Fix linting.

85dab75

Fix workflows hopefully.

b18618b

Removed confusing comment.

ed428c7

Add darwin arm64 lib

0dd51a2

Added helper function to reader to perform a wildcard query with the …

49d61fb

…max time interval.

LinZhihao-723 approved these changes May 9, 2024

View reviewed changes

LinZhihao-723 changed the title ~~Support all CLP encoding/decoding/searching FFI functionality and large scale refactor/rewrite~~ Add support for all CLP FFI functionality and refactor existing code: May 10, 2024

LinZhihao-723 merged commit 238b9a9 into main May 10, 2024
8 checks passed

davidlion deleted the beta branch May 10, 2024 16:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for all CLP FFI functionality and refactor existing code: #3

Add support for all CLP FFI functionality and refactor existing code: #3

davidlion commented Apr 10, 2024 •

edited by LinZhihao-723

Loading

LinZhihao-723 left a comment

LinZhihao-723 left a comment

LinZhihao-723 left a comment

LinZhihao-723 left a comment

LinZhihao-723 left a comment

LinZhihao-723 left a comment

LinZhihao-723 left a comment

LinZhihao-723 left a comment

LinZhihao-723 left a comment •

edited

Loading

LinZhihao-723 left a comment

LinZhihao-723 Apr 28, 2024

LinZhihao-723 Apr 29, 2024

davidlion May 7, 2024

LinZhihao-723 May 8, 2024

davidlion May 8, 2024

LinZhihao-723 May 9, 2024

LinZhihao-723 left a comment

LinZhihao-723 left a comment

davidlion commented May 10, 2024

Add support for all CLP FFI functionality and refactor existing code: #3

Add support for all CLP FFI functionality and refactor existing code: #3

Conversation

davidlion commented Apr 10, 2024 • edited by LinZhihao-723 Loading

Description

Validation performed

LinZhihao-723 left a comment

Choose a reason for hiding this comment

LinZhihao-723 left a comment

Choose a reason for hiding this comment

LinZhihao-723 left a comment

Choose a reason for hiding this comment

LinZhihao-723 left a comment

Choose a reason for hiding this comment

LinZhihao-723 left a comment

Choose a reason for hiding this comment

LinZhihao-723 left a comment

Choose a reason for hiding this comment

LinZhihao-723 left a comment

Choose a reason for hiding this comment

LinZhihao-723 left a comment

Choose a reason for hiding this comment

LinZhihao-723 left a comment • edited Loading

Choose a reason for hiding this comment

LinZhihao-723 left a comment

Choose a reason for hiding this comment

LinZhihao-723 Apr 28, 2024

Choose a reason for hiding this comment

LinZhihao-723 Apr 29, 2024

Choose a reason for hiding this comment

davidlion May 7, 2024

Choose a reason for hiding this comment

LinZhihao-723 May 8, 2024

Choose a reason for hiding this comment

davidlion May 8, 2024

Choose a reason for hiding this comment

LinZhihao-723 May 9, 2024

Choose a reason for hiding this comment

LinZhihao-723 left a comment

Choose a reason for hiding this comment

LinZhihao-723 left a comment

Choose a reason for hiding this comment

davidlion commented May 10, 2024

davidlion commented Apr 10, 2024 •

edited by LinZhihao-723

Loading

LinZhihao-723 left a comment •

edited

Loading