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

feat: support Tencent Cloud Log Service #7593

Merged
merged 26 commits into from
Aug 25, 2022
Merged

feat: support Tencent Cloud Log Service #7593

merged 26 commits into from
Aug 25, 2022

Conversation

ychensha
Copy link
Contributor

@ychensha ychensha commented Aug 2, 2022 •
edited by spacewander
Loading

Description

product doc

apisix log util is more powerfull than nginx access log, and it's useful for debugging and replaying later.

Fixes #7592

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@ychensha
add tencent-cloud-cls
bd8c579 
add tencent-cloud-cls

add tencent-cloud-cls

add tencent-cloud-cls

add tencent-cloud-cls

add tencent-cloud-cls
@ychensha ychensha changed the title [WIP]support Tencent Cloud Log Service support Tencent Cloud Log Service Aug 2, 2022
@soulbird soulbird changed the title support Tencent Cloud Log Service feat: support Tencent Cloud Log Service Aug 3, 2022
@soulbird
Copy link
Contributor

soulbird commented Aug 3, 2022

Thanks for your contribution. But please add a license to each file and add test cases first.

@ychensha
Copy link
Contributor Author

ychensha commented Aug 3, 2022

any doc for testing? I have no exp for env setup & PERL testing framework...

@soulbird
Copy link
Contributor

soulbird commented Aug 3, 2022

any doc for testing? I have no exp for env setup & PERL testing framework...

See: https://github.com/apache/apisix/blob/master/docs/en/latest/internal/testing-framework.md

@ychensha
Copy link
Contributor Author

ychensha commented Aug 3, 2022

any doc for testing? I have no exp for env setup & PERL testing framework...

See: https://github.com/apache/apisix/blob/master/docs/en/latest/internal/testing-framework.md

tried.. I work on Mac, and now trapped in etcd configuration for more tesing case

@ychensha
add ut
69e952b 
add ut
@ychensha ychensha force-pushed the feature/cls-logger branch 2 times, most recently from 78579de to 2ab9eef Compare August 4, 2022 01:23
@ychensha
fix lint
f57cc30 
fix lint

fix lint
spacewander
spacewander reviewed Aug 4, 2022
View reviewed changes
apisix/plugins/tencent-cloud-cls.lua Outdated Show resolved Hide resolved
apisix/plugins/tencent-cloud-cls.lua Outdated Show resolved Hide resolved
apisix/plugins/tencent-cloud-cls.lua Outdated Show resolved Hide resolved
apisix/plugins/tencent-cloud-cls/cls-sdk.lua Outdated Show resolved Hide resolved
apisix/plugins/tencent-cloud-cls/cls-sdk.lua Outdated Show resolved Hide resolved
apisix/plugins/tencent-cloud-cls/cls-sdk.lua Show resolved Hide resolved
apisix/plugins/tencent-cloud-cls/cls-sdk.lua Outdated Show resolved Hide resolved
apisix/plugins/tencent-cloud-cls/cls-sdk.lua Outdated Show resolved Hide resolved
apisix/plugins/tencent-cloud-cls/cls-sdk.lua Show resolved Hide resolved
apisix/plugins/tencent-cloud-cls/cls-sdk.lua Outdated Show resolved Hide resolved
tzssangglass
tzssangglass reviewed Aug 10, 2022
View reviewed changes
apisix/plugins/tencent-cloud-cls.lua Outdated
properties = {
cls_host = { type = "string" },
cls_topic = { type = "string" },
-- https://console.cloud.tencent.com/capi
Copy link
Member

Choose a reason for hiding this comment

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

what is this link for?

apisix/plugins/tencent-cloud-cls.lua Outdated Show resolved Hide resolved
@tzssangglass
Copy link
Member

I have a question: Is it that we can't do the test case to verify that this plugin is working correctly?
Because it requires a Tencent account and to make sure that this account is always available.

@spacewander
Copy link
Member

I have a question: Is it that we can't do the test case to verify that this plugin is working correctly? Because it requires a Tencent account and to make sure that this account is always available.

We can inject a hook or mock into the code? Like:

apisix/t/plugin/zipkin2.t

Line 35 in b3d4b46

my $extra_init_by_lua = <<_EOC_;

apisix/t/plugin/opentelemetry.t

Line 553 in b3d4b46

--- extra_init_by_lua

@ychensha
Copy link
Contributor Author

I have same question too. But same question can be applie to other plugins that have env matters

@ychensha
Copy link
Contributor Author

I have same question too. But same question can be applie to other plugins that have env matters

@ychensha ychensha closed this Aug 10, 2022
@ychensha
Copy link
Contributor Author

ychensha commented Aug 10, 2022 •
edited
Loading

@spacewander @tzssangglass after code related conversations closed, I will do the function test again for the related code modification.

spacewander
spacewander reviewed Aug 18, 2022
View reviewed changes
Copy link
Member

@spacewander spacewander left a comment

Choose a reason for hiding this comment

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

Some comments need to be handled:
https://github.com/apache/apisix/pull/7593/files#r947501015
https://github.com/apache/apisix/pull/7593/files#r937476352
https://github.com/apache/apisix/pull/7593/files#r947500216

apisix/plugins/tencent-cloud-cls/cls-sdk.lua Outdated Show resolved Hide resolved
apisix/plugins/tencent-cloud-cls/cls-sdk.lua Outdated Show resolved Hide resolved
@tzssangglass
Copy link
Member

@ychensha pls don't use force-push to update code, or reviews' comments may be ignore.

spacewander
spacewander reviewed Aug 19, 2022
View reviewed changes
t/plugin/tencent-cloud-cls.t Show resolved Hide resolved
t/plugin/tencent-cloud-cls.t Outdated Show resolved Hide resolved
t/plugin/tencent-cloud-cls.t Outdated Show resolved Hide resolved
t/plugin/tencent-cloud-cls.t Outdated Show resolved Hide resolved
spacewander
spacewander reviewed Aug 22, 2022
View reviewed changes
apisix/plugins/tencent-cloud-cls/cls-sdk.lua Show resolved Hide resolved
apisix/plugins/tencent-cloud-cls/cls-sdk.lua Outdated Show resolved Hide resolved
t/plugin/tencent-cloud-cls.t Outdated Show resolved Hide resolved
t/plugin/tencent-cloud-cls.t Outdated Show resolved Hide resolved
t/plugin/tencent-cloud-cls.t Show resolved Hide resolved
@spacewander
Copy link
Member

Let's merge master to make CI pass.

spacewander
spacewander previously approved these changes Aug 23, 2022
View reviewed changes
Copy link
Member

@spacewander spacewander left a comment

Choose a reason for hiding this comment

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

Approve except https://github.com/apache/apisix/pull/7593/files#r941961486
As this PR is big enough, let's open another PR for the doc.

@spacewander
Copy link
Member

As this PR is big enough, let's open another PR for the doc.

@spacewander spacewander merged commit 3c07574 into apache:master Aug 25, 2022
Liu-Junlin pushed a commit to Liu-Junlin/apisix that referenced this pull request Nov 4, 2022
@ychensha @Liu-Junlin
feat: support Tencent Cloud Log Service (apache#7593)
dfa29ce
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soulbird soulbird soulbird left review comments

@spacewander spacewander spacewander approved these changes

@tzssangglass tzssangglass tzssangglass approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: Support Tencent Cloud Log Service
4 participants
@ychensha @soulbird @tzssangglass @spacewander