Skip to content

Add postgresql agent sql query param show #3695

Merged
wu-sheng merged 20 commits intomasterfrom
unknown repository
Oct 28, 2019
Merged

Add postgresql agent sql query param show #3695
wu-sheng merged 20 commits intomasterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Oct 23, 2019 •
edited by ghost
Loading

Please answer these questions before submitting pull request

  • Why submit this pull request?

  • Bug fix

  • New feature provided

  • Improve performance

  • Related issues

Bug fix

  • Bug description.

  • How to fix?

New feature or improvement

  • Describe the details and related test reports.

@ghost ghost mentioned this pull request Oct 23, 2019
Closed
3 tasks
@kezhenxu94 kezhenxu94 added agent Language agent related. feature New feature plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor. labels Oct 23, 2019
kezhenxu94
kezhenxu94 requested changes Oct 23, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

@Aderm have you tested locally? seems there're apparent errors, see comments inline.

We hope the contributors should at least test by themselves first to guarantee the correctness, instead of depending on the committers/PMCs, and, it would be better to track back to the similar issue/PR to get the context and reason why we wrote such codes, for this PR, it can be tracked back to #2846 and you'll find the context in this review comment #2846 (comment)

Comment thread ...ommons/src/main/java/org/apache/skywalking/apm/plugin/jdbc/define/StatementEnhanceInfos.java Outdated
@ghost ghost changed the title Add postgresql agent sql query param show [WIP]Add postgresql agent sql query param show Oct 24, 2019
wu-sheng
wu-sheng reviewed Oct 24, 2019
View reviewed changes
Copy link
Copy Markdown
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.

Doc setup doc should update too. We need to guide user about how to use this. Ref MySql config.

@kezhenxu94 please review

@kezhenxu94
Copy link
Copy Markdown
Member

kezhenxu94 commented Oct 25, 2019

@Aderm ping me when you think it's ready

@ghost
Copy link
Copy Markdown
Author

ghost commented Oct 27, 2019

@kezhenxu94 add sql param is done. second i want to move some pg common in sub module postgresql-commons, what do you think?

kezhenxu94
kezhenxu94 requested changes Oct 27, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

Comments in last review are not addressed yet

Comment thread apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/conf/Config.java Outdated
Comment thread apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/conf/Config.java Outdated
Comment thread apm-sniffer/apm-sdk-plugin/postgresql-8.x-plugin/pom.xml Outdated
Comment thread ...ommons/src/main/java/org/apache/skywalking/apm/plugin/jdbc/define/StatementEnhanceInfos.java Outdated
Comment thread ...lugin/src/main/java/org/apache/skywalking/apm/plugin/jdbc/postgresql/util/ParameterUtil.java Outdated
kezhenxu94
kezhenxu94 reviewed Oct 27, 2019
View reviewed changes
Comment thread test/plugin/scenarios/postgresql-above9.4.1207-scenario/config/expectedData.yaml Outdated
kezhenxu94
kezhenxu94 reviewed Oct 27, 2019
View reviewed changes
Comment thread test/plugin/scenarios/postgresql-above9.4.1207-scenario/config/expectedData.yaml Outdated
@ghost
Copy link
Copy Markdown
Author

ghost commented Oct 27, 2019

@kezhenxu94 done

kezhenxu94
kezhenxu94 approved these changes Oct 28, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

LGTM

@wu-sheng wu-sheng changed the title [WIP]Add postgresql agent sql query param show Add postgresql agent sql query param show Oct 28, 2019
wu-sheng
wu-sheng approved these changes Oct 28, 2019
View reviewed changes
Copy link
Copy Markdown
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. Congrats for finally getting this landed into the master. Good work.

@wu-sheng wu-sheng added this to the 6.5.0 milestone Oct 28, 2019
@wu-sheng wu-sheng merged commit 7f69a73 into apache:master Oct 28, 2019
@ghost ghost deleted the sql branch October 28, 2019 01:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ascrutae ascrutae Awaiting requested review from ascrutae

@IanCao IanCao Awaiting requested review from IanCao

@zhaoyuguang zhaoyuguang Awaiting requested review from zhaoyuguang

Assignees

No one assigned

Labels

agent Language agent related. feature New feature plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor.

Projects

None yet

Milestone

6.5.0

Development

Successfully merging this pull request may close these issues.

3 participants

@kezhenxu94 @wu-sheng @adermxzs