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

Align gRPC server status code to span status code #144

Merged

Conversation

@CLAassistant
Copy link

CLAassistant commented Nov 3, 2023

CLA assistant check
All committers have signed the CLA.

@dragon3 dragon3 force-pushed the align-grpc-status-code-to-span-status-code branch from 60704dd to 8b5cd78 Compare November 3, 2023 07:21
@akshayjshah
Copy link
Member

Thank you for the contribution, @dragon3! This repository - and the OpenTelemetry RPC semantic conventions - are in a strange state right now, because Google's gRPC projects are planning to directly support OpenTelemetry, but they don't plan to follow the OTel RPC semantic conventions. (See grpc/proposal#380 for details.) If Google's gRPC implementations aren't following the proposed semantic conventions, there may not be much value in us following it either.

Before merging this PR, we'd like to discuss the best path forward a bit more among the maintainers, followed by a more open discussion on a GitHub issue. Let's keep this PR open until we make a decision.

@dragon3
Copy link
Contributor Author

dragon3 commented Nov 13, 2023

@akshayjshah
Thank you for the reply.

I didn't know the situation and it's interesting 😅 I also find grpc/proposal#389
I hope Google's gRPC implementation will follow the OTel semantic conventions for both metrics, and tracing ( also logging in the future )

Let me explain my motivation to create this PR.

I develop and maintain both gRPC and ConnectRPC services every day for my company. Of course, I use OpenTelemetry for both gRPC and ConnectRPC services observability. That's great but the difference in the span status code between them is a bit hassle.

For example, in gRPC NOT_FOUND span status is Unset, it's good to me to monitor the service's error rate and alerting. But in ConnectRPC the status is Error, it's not great to me because it's expected one and I don't want to count as error, then hard to monitor the service.

I'd like to unify the span status ( follow the current gRPC's status ), that's my motivation.

Anyway, I'm looking forward to your decision. Thank you!

@jhump
Copy link
Member

jhump commented Dec 8, 2023

@dragon3, I think we've come around to wanting to merge this change. It looks like it will only bring us closer to Google's gRPC implementation, which is the desired direction. (The main initial concern was trying to adhere to an Otel spec that Google's gRPC implementation does not follow.)

Do you mind rebasing this pull request?

@dragon3
Copy link
Contributor Author

dragon3 commented Dec 9, 2023

@jhump
Thank you for the comment. I've just rebased this pull request. Please take a look when you get a chance.

@dragon3 dragon3 force-pushed the align-grpc-status-code-to-span-status-code branch from b55de1c to 10beddf Compare December 9, 2023 01:07
@jhump
Copy link
Member

jhump commented Dec 11, 2023

@dragon3, can you address the lint issues? BTW, you can run the linter locally, before pushing commits to GitHub, by running make lint.

@dragon3
Copy link
Contributor Author

dragon3 commented Dec 11, 2023

@jhump
I've fixed the lint errors with make lint 😄 Please run CI and check the result again 🙏

@jhump jhump merged commit 07e7942 into connectrpc:main Dec 12, 2023
5 checks passed
@jhump
Copy link
Member

jhump commented Dec 12, 2023

Thanks for the contribution, @dragon3

@dragon3 dragon3 deleted the align-grpc-status-code-to-span-status-code branch December 12, 2023 23:38
@dragon3
Copy link
Contributor Author

dragon3 commented Dec 12, 2023

@jhump
Thank you for reviewing and merging 👍

Could you create a new tag including this change?

@jhump
Copy link
Member

jhump commented Dec 13, 2023

Could you create a new tag including this change?

@dragon3, I think we can do that later this week. I'm hoping to get one more change (maybe two) merged before cutting the next release.

@dragon3
Copy link
Contributor Author

dragon3 commented Jan 11, 2024

@jhump
Sorry for asking again, but could you create a new tag?
Thank you!

@jhump
Copy link
Member

jhump commented Jan 11, 2024

@dragon3, apologies for the delay, and thanks for pinging me about it. v0.7.0 is now released.

@dragon3
Copy link
Contributor Author

dragon3 commented Jan 11, 2024

@jhump Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adhere to gRPC error code statuses
4 participants