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.

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

Support span#error in toolkit #2605

Merged
merged 23 commits into from
May 10, 2019
Merged

Support span#error in toolkit #2605

merged 23 commits into from
May 10, 2019

Conversation

IanCao
Copy link
Contributor

@IanCao IanCao commented May 6, 2019

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.

@wu-sheng wu-sheng changed the title fix #2599 Support span#error in toolkit May 6, 2019
@wu-sheng wu-sheng added this to the 6.2.0 milestone May 6, 2019
@wu-sheng wu-sheng added agent Language agent related. plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor. labels May 6, 2019
@wu-sheng wu-sheng requested a review from ascrutae May 6, 2019 07:41
@wu-sheng
Copy link
Member

wu-sheng commented May 6, 2019

@ascrutae Do we need test case for this?

@wu-sheng
Copy link
Member

wu-sheng commented May 6, 2019

Support #2599

@ascrutae
Copy link
Member

ascrutae commented May 6, 2019

@IanCao I suggest that to add error message or an exception in ActiveSpan#error API, Because the user will confused why this span occurs an exception when the span without any error message or an exception.

@wu-sheng
Copy link
Member

wu-sheng commented May 6, 2019

I suggest that to add error message or an exception in ActiveSpan#error API, Because the user will confused why this span occurs an exception when the span without any error message or an exception.

@ascrutae Exception is a log API, do you suggest to add that?

@coveralls
Copy link

coveralls commented May 6, 2019

Coverage Status

Coverage increased (+0.05%) to 16.413% when pulling 292c10b on IanCao:master into ed78dab on apache:master.

@IanCao
Copy link
Contributor Author

IanCao commented May 6, 2019

I suggest that to add error message or an exception in ActiveSpan#error API, Because the user will confused why this span occurs an exception when the span without any error message or an exception.

@ascrutae Exception is a log API, do you suggest to add that?

i have added three error methods and test case

   public static void error() {
    }
    public static void error(String key, String errorMsg) {
    }
    public static void error(Throwable throwable) {
    }

plz review~

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.

cc @ascrutae comments inline.

@IanCao
Copy link
Contributor Author

IanCao commented May 7, 2019

@wu-sheng please review

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.

inline.

@wu-sheng
Copy link
Member

wu-sheng commented May 7, 2019

Application toolkit document should update, and add an integration test case for this?

@IanCao
Copy link
Contributor Author

IanCao commented May 7, 2019

Application toolkit document should update, and add an integration test case for this?

ok

@IanCao
Copy link
Contributor Author

IanCao commented May 9, 2019

Application toolkit document should update, and add an integration test case for this?

doc and integration test case have added

@SkyWalkingRobot
Copy link

Here is the test report and validate logs

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.

Document change suggestions inline.

@wu-sheng wu-sheng merged commit dd9d178 into apache:master May 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent Language agent related. plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants