Message events: use counters instead of hexdecimal value #446
Conversation
mayurkale22
requested review from
draffensperger and
justindsmith
as code owners
|
/cc @HollandBen |
| @@ -84,6 +84,8 @@ export class GrpcPlugin extends BasePlugin { | |||
| static readonly ATTRIBUTE_GRPC_STATUS_CODE = 'grpc.status_code'; | |||
| static readonly ATTRIBUTE_GRPC_ERROR_NAME = 'grpc.error_name'; | |||
| static readonly ATTRIBUTE_GRPC_ERROR_MESSAGE = 'grpc.error_message'; | |||
| private sentSeqId = 1; | |||
There was a problem hiding this comment.
My reading of the specs is that these counters are supposed to be per-request so that the messages for a given request can be correlated between services. Would it be possible to handle these on a per request basis?
There was a problem hiding this comment.
Ah! You're right! I checked in other libraries. In Go, id is 0 for all requests. In Python, wraps a request or response iterator to add message events to the span. Looks like Java has most accurate implementation, But I am not sure how it translate previous hop count (seq) on wire. I will talk to @songy23 and @rghetia to understand it better.
There was a problem hiding this comment.
I had a word with @rghetia, so the seq number counter is for single request and not across multiple services. In case of unary calls only one sent and one received message will be recorded for both client and server spans. I will add support for streaming calls in another PR. I have updated the PR with this change and also changed id type from string to number. I know this is a breaking change, but this API - addMessageEvent() is mostly used internally.
mayurkale22
force-pushed
the
message_id
branch
2 times, most recently
from
ec69787
to
9ac6c11
Compare
Codecov Report
@@ Coverage Diff @@
## master #446 +/- ##
==========================================
- Coverage 94.71% 94.68% -0.03%
==========================================
Files 150 150
Lines 9743 9765 +22
Branches 734 735 +1
==========================================
+ Hits 9228 9246 +18
- Misses 515 519 +4
Continue to review full report at Codecov.
|
This is mostly used in the internal APIs and it won't impact the end users.
Fixes #442
As per the Specs, The
MessageIdmust be calculated as two different counters starting from 1 one for sent messages and one for received message. This way we guarantee that the values will be consistent between different implementations.To avoid breaking changes, I preferred to keepstringtype for id field. I am happy to change that or add support for bothstring|number.