Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[#noissue] Handle ByteString annotation values of protobuf #9705

Merged
merged 1 commit into from
May 2, 2023

Conversation

kojandy
Copy link
Contributor

@kojandy kojandy commented Feb 16, 2023

Since bytes in protobuf model corresponds to com.google.protobuf.ByteString instead of byte[] in Java, I've added logic to convert ByteString to byte[] when ByteString-typed annotation value is received.

@kojandy kojandy self-assigned this Feb 16, 2023
@emeroad emeroad added this to the 2.6.0 milestone Feb 16, 2023
@codecov
Copy link

codecov bot commented Feb 16, 2023

Codecov Report

Merging #9705 (9e655c2) into master (3702da1) will increase coverage by 0.09%.
The diff coverage is 0.00%.

❗ Current head 9e655c2 differs from pull request most recent head 6dd9f90. Consider uploading reports for the commit 6dd9f90 to get more accurate results

@@             Coverage Diff              @@
##             master    #9705      +/-   ##
============================================
+ Coverage     38.75%   38.84%   +0.09%     
+ Complexity    11918    11901      -17     
============================================
  Files          3510     3538      +28     
  Lines         94258    93975     -283     
  Branches      10591    10494      -97     
============================================
- Hits          36529    36507      -22     
+ Misses        54553    54293     -260     
+ Partials       3176     3175       -1     
Impacted Files Coverage Δ
...p/pinpoint/common/server/bo/AnnotationFactory.java 28.12% <ø> (ø)
...t/common/server/bo/grpc/GrpcAnnotationHandler.java 11.53% <0.00%> (-0.47%) ⬇️

... and 382 files with indirect coverage changes

@CLAassistant
Copy link

CLAassistant commented Feb 16, 2023

CLA assistant check
All committers have signed the CLA.

@smilu97
Copy link
Contributor

smilu97 commented Feb 20, 2023

I'm sorry that I didn't realized it last time, but I'm not sure if it's ok to make AnnotationFactory grpc-dependent...?

I think we can make another GrpcAnnotationFactory which extends AnnotationFactory, and use it in GrpcSpanBinder

I found we already have GrpcAnnotationHandler::buildCustomAnnotationValue.

@kojandy
Copy link
Contributor Author

kojandy commented Feb 20, 2023

Yup, as @emeroad already pointed it out on last weekly meeting, I will move relating-logic to the location you've suggested. and I guess this PR won't be merged until 2.6.0 release when the actual use of the type bytes in protobuf model is introduced, right? Or can I merge this PR regardless of the milestone label?

@smilu97
Copy link
Contributor

smilu97 commented Feb 20, 2023

I didn't catch that conversation was about this XD

Anyway, the milestone labels are maybe just for some summarizing stuffs like release note, and pointing out the features related to those commits are experimental. I think it is totally fine to merge some branches now, and do some experiments on the dev cluster

@kojandy
Copy link
Contributor Author

kojandy commented Feb 20, 2023

Oh, I see. Thank you for the guideline. I will fix it right away then!

@smilu97
Copy link
Contributor

smilu97 commented Feb 21, 2023

Sorry for misguiding.

Setting milestone means the PR requires to be merged after the version exactly as you said.

@kojandy kojandy force-pushed the handle-bytestring branch from 9e655c2 to 6dd9f90 Compare May 2, 2023 06:20
@kojandy kojandy merged commit d62304b into pinpoint-apm:master May 2, 2023
@kojandy kojandy deleted the handle-bytestring branch May 2, 2023 06:21
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants