core: Add new OpenCensus tags for method names #5601
Conversation
|
Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement. After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.
Regards, |
TheMarex
force-pushed
the
fix/deprecated_oc_tag_method
branch
from
960f3df
to
232ef58
Compare
|
Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement. After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.
Regards, |
TheMarex
changed the title
There was a problem hiding this comment.
LGTM assuming this doesn't cause performance issue.
Otherwise, consider just removing put(DeprecatedCensusConstants.RPC_METHOD, methodTag) like I mentioned in #5593 (comment).
|
@carl-mastrangelo has been profiling gRPC with Census. Unless there has been performance optimization in Census tagging, I don't expect the performance overhead to be different from the previous results. It may be better to first do the mapping mentinoed in #5593 (comment) under the hood, then migrate gRPC to the new tags. |
Make sense to me, added the mapping in census-instrumentation/opencensus-java#1854. With that those who use the deprecated constants will continue getting the correct metrics. |
|
@songy23 with the change in census-instrumentation/opencensus-java#1854 in mind, should I adapt this PR to only set the non-deprecated constant? My understanding then would be that this would mean all following gRPC versions would depend on a OpenCensus version > 0.19 - is that correct? Thank you for looking at this so fast. 👍 Sorry about the CLA noise, figuring this out with my org. |
If there's no performance regression, I would still like to set both the deprecated and new tags. That way we can ensure both old and new RPC metrics work for all OpenCensus versions. On the other hand if only the new tags are set, users who depend on a lower version of OpenCensus (<0.21) and use the old RPC metrics will lose data of the old "method"/"status" tag. They'll have to either migrate to the new RPC metrics, or upgrade OpenCensus to 0.21.
That's right, census-instrumentation/opencensus-java#1854 will be released in OpenCensus 0.21.0. After the release I'll update gRPC to use the newer version. |
gRPC does not support users downgrading versions of our dependencies. If we require opencensus 0.21, the user can use opencensus 0.21 or newer. |
That's great, then please hold off this PR a bit until I upgraded the OC version. |
This fixes issue grpc#5593 by adding the tag `grpc_{server,client}_method` to all default measurements. It restores compatibility with OpenCensus >=0.13 without using deprecated views.
TheMarex
force-pushed
the
fix/deprecated_oc_tag_method
branch
from
9df7146
to
e1b476a
Compare
|
Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement. After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.
Regards, |
|
@TheMarex please fix build failures. |
Fixes grpc#5593 and supersedes grpc#5601 Now that census-instrumentation/opencensus-java#1854 has been merged & released as 0.21.0. We can start using the method & status tags. Background: Opencensus introduced new tags for status and method (census-instrumentation/opencensus-java#1115). The old views that used those tags were deprecated and new views were created that used the new tags. However grpc-java wasn't updated to use the new tags due to concern of breaking existing metrics. This resulted in the old views being deprecated while the new views were broken (goomics grpc#50). census-instrumentation/opencensus-java#1854 added a compatibility layer to opencensus that would remap new tags to old tags for old views. This should unblock grpc to switching to the new tags while allowing old views to still be populated. That commit was released as part of opencensus 0.21, which grpc currently uses
|
I propose to close this PR in favor of #5996 |
|
@igorbernstein2 thanks for picking this up again. I agree, please use #5996 in favor of this. FYI I wasn't able to figure out the CLA thing and Linux Foundation is unresponsive. |
Fixes #5593 and supersedes #5601 Now that census-instrumentation/opencensus-java#1854 has been merged & released as 0.21.0. We can start using the method & status tags. Background: Opencensus introduced new tags for status and method (census-instrumentation/opencensus-java#1115). The old views that used those tags were deprecated and new views were created that used the new tags. However grpc-java wasn't updated to use the new tags due to concern of breaking existing metrics. This resulted in the old views being deprecated while the new views were broken (goomics #50). census-instrumentation/opencensus-java#1854 added a compatibility layer to opencensus that would remap new tags to old tags for old views. This should unblock grpc to switching to the new tags while allowing old views to still be populated. That commit was released as part of opencensus 0.21, which grpc currently uses
This PR fixes #5593 by adding support for the new gRPC method to the
CensusModule.I think it will be necessary to have this overlap for at least one versions to give clients a chance to upgrade without breaking changes. Since gRPC already assumes OpenCensus >0.17, the required migration steps for a client would be:
OpenCensusgRPCViews provided byopencensus-javathat use these new tagsViews