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.

Sign up for GitHub

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

Jump to bottom

Add API && Plugin framework registry #5

Merged
merged 25 commits into from Dec 3, 2020
Merged

Add API && Plugin framework registry #5

merged 25 commits into from Dec 3, 2020

Conversation

EvanLjp
Copy link
Member

@EvanLjp EvanLjp commented Nov 27, 2020

Add API && Plugin framework registry. Please review the reasonableness of these definitions

@EvanLjp EvanLjp added the enhancement New feature or request label Nov 27, 2020
@EvanLjp EvanLjp added this to the 0.1.0 milestone Nov 27, 2020
wu-sheng
wu-sheng reviewed Nov 27, 2020
View reviewed changes
Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

My major question would be,

  1. Which interface matches the filter in the design doc? I want to have a more concept consistent between codes and doc. Please provide a map in the comments if necessary.
  2. Event in the doc should be the data of the processor. From my understanding, there should be no explicit boundary between input and output, because every output could be another filter's input. I am curious how you plan to use the InputEvent and OutputEvent.
  3. Why Output named as BatchOutputEvents? Batch or no batch depends on filter works. You could have several inputs to one output, vice versa.

EvanLjp
EvanLjp commented Nov 27, 2020
View reviewed changes
internal/pkg/api/plugin.go Outdated Show resolved Hide resolved
@EvanLjp
Copy link
Member Author

EvanLjp commented Nov 27, 2020 •
edited

3. Why Output named as BatchOutputEvents? Batch or no batch depends on filter works. You could have several inputs to one output, vice versa.

The Batch is pointed to the BatchBuffer context in the design doc.
The core structure of BatchEvents is map[string][]Event, which has many slices partition by event name.
In the first release version, Satellite would more forward rather than processing. So, the Sender would be independent in Log, Metrics, and Trace. In the future, some advanced features would break the current mod, such as extract Metrics from Log. SO, The Sender Layer would be a shared layer to avoid make duplicate connections.

@EvanLjp
Copy link
Member Author

EvanLjp commented Nov 27, 2020 •
edited

2. Event in the doc should be the data of the processor. From my understanding, there should be no explicit boundary between input and output, because every output could be another filter's input. I am curious how you plan to use the InputEvent and OutputEvent.

Event is a global context, all Events were allowed to transfer in Satellite. InputEvent is a smaller concept for Gatherer. Because Gatherer would put the data in Queue, InputEvent guarantees that Queue can be replaced in different plug-ins. It's up to the plug-in to decide whether to take the original look or serialize to bytes.

wu-sheng
wu-sheng reviewed Nov 28, 2020
View reviewed changes
Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

Put more comments.

internal/pkg/api/plugin.go Outdated Show resolved Hide resolved
internal/pkg/api/plugin.go Outdated Show resolved Hide resolved
internal/pkg/api/plugin.go Outdated Show resolved Hide resolved
internal/pkg/api/plugin.go Outdated Show resolved Hide resolved
internal/pkg/api/plugin.go Outdated Show resolved Hide resolved
internal/pkg/api/plugin.go Outdated Show resolved Hide resolved
wu-sheng
wu-sheng previously approved these changes Dec 1, 2020
View reviewed changes
Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

LGTM.

gxthrj
gxthrj previously approved these changes Dec 1, 2020
View reviewed changes
@wu-sheng
Copy link
Member

wu-sheng commented Dec 1, 2020

@surlymo Could you recheck and confirm?

surlymo
surlymo reviewed Dec 1, 2020
View reviewed changes
internal/pkg/api/forwarder.go Outdated Show resolved Hide resolved
internal/pkg/api/event.go Outdated Show resolved Hide resolved
internal/satellite/event/struectured_event.go Outdated Show resolved Hide resolved
surlymo
surlymo requested changes Dec 1, 2020
View reviewed changes
internal/satellite/event/unstruectured_event.go Outdated Show resolved Hide resolved
internal/satellite/event/unstruectured_event.go Outdated Show resolved Hide resolved
internal/satellite/registry/registry.go Outdated Show resolved Hide resolved
internal/satellite/registry/registry.go Outdated Show resolved Hide resolved
internal/satellite/registry/registry.go Outdated Show resolved Hide resolved
internal/satellite/event/struectured_event.go Outdated Show resolved Hide resolved
@surlymo surlymo self-requested a review December 1, 2020 13:55
@EvanLjp EvanLjp dismissed stale reviews from gxthrj and wu-sheng via c5ac6ed December 1, 2020 16:07
surlymo
surlymo requested changes Dec 2, 2020
View reviewed changes
Copy link

@surlymo surlymo left a comment

Choose a reason for hiding this comment

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

registry module should be designed further. try it plz.

internal/satellite/registry/registry.go Outdated Show resolved Hide resolved
internal/satellite/registry/registry.go Outdated Show resolved Hide resolved
internal/satellite/registry/registry.go Outdated Show resolved Hide resolved
surlymo
surlymo reviewed Dec 2, 2020
View reviewed changes
internal/pkg/api/plugin.go Outdated Show resolved Hide resolved
internal/satellite/registry/registry.go Outdated Show resolved Hide resolved
internal/satellite/registry/registry.go Outdated Show resolved Hide resolved
@EvanLjp
Copy link
Member Author

EvanLjp commented Dec 2, 2020

@surlymo Thx for review again. Both support pointer register and value register.
image

@EvanLjp EvanLjp requested a review from surlymo December 2, 2020 12:42
surlymo
surlymo previously approved these changes Dec 2, 2020
View reviewed changes
Copy link

@surlymo surlymo left a comment

Choose a reason for hiding this comment

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

LGTM

@surlymo surlymo closed this Dec 2, 2020
@surlymo surlymo reopened this Dec 2, 2020
wu-sheng
wu-sheng reviewed Dec 2, 2020
View reviewed changes
Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

Why named package like defineclient? What does this mean?

@EvanLjp
Copy link
Member Author

EvanLjp commented Dec 2, 2020 •
edited

Why named package like defineclient? What does this mean?

move plugin API to plugin dir ? I do this to avoid packet conflicts ,Do you have any other naming suggestions? could I merge?

EvanLjp
EvanLjp commented Dec 2, 2020
View reviewed changes
docs/project_structue.md Outdated
- internal/satellite: The core of Satellite.
- plugins: Contains all plugins.
- plugins/{type}: Contains the plugins of this {type}. Satellite has 6 plugin types, which are collector, queue, parser, filter, client, and forward.
- plugins/{type}/define{type}: Contains the plugin define.
Copy link
Member Author

Choose a reason for hiding this comment

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

move API defines to plugins dir

Copy link
Member

Choose a reason for hiding this comment

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

Why need duplicate {type}? And define is a verb, usually, a package name is a noun.

Copy link
Member Author

Choose a reason for hiding this comment

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

just to avoid package conflicts, let me try again.

Copy link
Member

Choose a reason for hiding this comment

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

What conflict?

Copy link
Member Author

Choose a reason for hiding this comment

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

already fixed

Copy link
Member Author

Choose a reason for hiding this comment

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

What conflict?

It's my mistake

Copy link
Member Author

Choose a reason for hiding this comment

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

could I merge it? According to chao's advice, the plugin was decoupled.

wu-sheng
wu-sheng approved these changes Dec 3, 2020
View reviewed changes
Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

Merging.

@wu-sheng wu-sheng merged commit 3bf78de into main Dec 3, 2020
@wu-sheng wu-sheng deleted the plugin-framework branch December 3, 2020 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wu-sheng wu-sheng wu-sheng approved these changes

@gxthrj gxthrj gxthrj left review comments

@surlymo surlymo surlymo left review comments

@arugal arugal Awaiting requested review from arugal

@moonming moonming Awaiting requested review from moonming

@nic-chen nic-chen Awaiting requested review from nic-chen

@hanahmily hanahmily Awaiting requested review from hanahmily

@kezhenxu94 kezhenxu94 Awaiting requested review from kezhenxu94

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
0.1.0
6 participants
@EvanLjp @wu-sheng @hanahmily @gxthrj @surlymo @kezhenxu94