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 in error status when http code >= 400, and add trace#Error in toolkit API #212

Merged
merged 14 commits into from
Dec 25, 2024

Conversation

alexwn
Copy link
Contributor

@alexwn alexwn commented Dec 22, 2024

1、add logic when http status code >= 400, set span isError is true by span.Error method
2、add new method trace.Error for user set san isError to true by self

@wu-sheng wu-sheng changed the title Bugfix #12882 Support span in error status when http code >= 400, and add trace#Error in toolkit API Dec 22, 2024
@wu-sheng wu-sheng added the enhancement New feature or request label Dec 22, 2024
@wu-sheng wu-sheng added this to the 0.6.0 milestone Dec 22, 2024
@mrproliu
Copy link
Contributor

Also, please update the CHANGES.md, thanks!

1、Features:support manually set span error in the toolkit.
2、Bug Fixes:Fix not set span error when http status code >= 400
@alexwn
Copy link
Contributor Author

alexwn commented Dec 22, 2024

Also, please update the CHANGES.md, thanks!
Done

@wu-sheng
Copy link
Member

@alexwn Are you still working on this?

@alexwn
Copy link
Contributor Author

alexwn commented Dec 24, 2024

@alexwn Are you still working on this?
yes, do you have any question? what else should I do?

… method is deleted

2、change gin excepted data /notFound isError=true
@wu-sheng
Copy link
Member

No, there isn't anything to ask. I am just checking whether the CI fix has any blocks.

@alexwn
Copy link
Contributor Author

alexwn commented Dec 24, 2024

No, there isn't anything to ask. I am just checking whether the CI fix has any blocks.

Yes, I saw it. I just changed the code and submitted it

@wu-sheng
Copy link
Member

Still failing. You may still need to make CI runnable locally, which is much easier to see the error and fix.

@alexwn
Copy link
Contributor Author

alexwn commented Dec 24, 2024

Still failing. You may still need to make CI runnable locally, which is much easier to see the error and fix.

image
image
I think it should be true, Should I change the gin plugin code?

@wu-sheng
Copy link
Member

If that case is using http component as well, then it is affected by your codes.

@alexwn
Copy link
Contributor Author

alexwn commented Dec 24, 2024

If that case is using http component as well, then it is affected by your codes.

entry span is not affected by my code, exist span is effected by my code. I just change two span's isError=true, exist span passed, entry span failed. So I should only change the exit span' isError=true and leave the entry span original?

@wu-sheng
Copy link
Member

If that case is using http component as well, then it is affected by your codes.

entry span is not affected by my code, exist span is effected by my code. I just change two span's isError=true, exist span passed, entry span failed. So I should only change the exit span' isError=true and leave the entry span original?

If you want to make changes to another plugin(gin), then you could make them aligned. It depends on you. Ideally, all HTTP RPC relative plugins should follow your new proposal.

@wu-sheng
Copy link
Member

@alexwn This change seems to impact other plugin tests as well, please fix them.

@alexwn
Copy link
Contributor Author

alexwn commented Dec 25, 2024

@alexwn This change seems to impact other plugin tests as well, please fix them.
kratosv2 except.yml data, one segment expect status_code=200, but actual no status_code, this segment's component is mux(5017), I saw the code of plugin(mux), it have logical to set the status_code, I have no idea how to fix it.
image
img_v3_02ht_4eb602c7-b33d-41d4-89fd-ce9dd92e932g

@mrproliu mrproliu merged commit ef154ab into apache:main Dec 25, 2024
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants