-
Notifications
You must be signed in to change notification settings - Fork 837
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
Add instrumentation tags to grpc metrics #1081
Add instrumentation tags to grpc metrics #1081
Conversation
grpc-common-spring-boot/src/main/java/net/devh/boot/grpc/common/util/Versions.java
Outdated
Show resolved
Hide resolved
…correct Versions.java file
[ EDIT: the following error has been fixed. ]
|
Some more work to be done... |
...boot-starter/src/main/java/net/devh/boot/grpc/client/metrics/MetricsClientStreamTracers.java
Outdated
Show resolved
Hide resolved
...boot-starter/src/main/java/net/devh/boot/grpc/client/metrics/MetricsClientStreamTracers.java
Outdated
Show resolved
Hide resolved
...boot-starter/src/main/java/net/devh/boot/grpc/client/metrics/MetricsClientStreamTracers.java
Outdated
Show resolved
Hide resolved
grpc-common-spring-boot/src/main/java/net/devh/boot/grpc/common/util/Constants.java.template
Outdated
Show resolved
Hide resolved
grpc-common-spring-boot/src/main/java/net/devh/boot/grpc/common/util/Constants.java.template
Outdated
Show resolved
Hide resolved
grpc-common-spring-boot/src/main/java/net/devh/boot/grpc/common/util/Constants.java.template
Outdated
Show resolved
Hide resolved
I haven't found a reason why these tags should be included. Is it part of the spec? |
grpc-common-spring-boot/src/main/java/net/devh/boot/grpc/common/util/Constants.java
Outdated
Show resolved
Hide resolved
grpc-common-spring-boot/src/main/java/net/devh/boot/grpc/common/util/Constants.java
Outdated
Show resolved
Hide resolved
grpc-common-spring-boot/src/main/java/net/devh/boot/grpc/common/util/Constants.java
Outdated
Show resolved
Hide resolved
Yes this is part of an internal spec that will help us distinguish where the metrics originate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure whether the tags should be included for everyone.
But implementation wise this looks good to me.
(Spring has a bean for adding tags to metrics).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Add two tags to the A66 metrics exported by the opencensus stats module. This will be picked up by the Cloud Monitoring pipeline and allow us to build dashboard off these tags. Similarly done in grpc-ecosystem/grpc-spring#1081
Add two tags to the A66 metrics exported by the MetricsStreamTracers