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

[protobuf] - encode zipcode as a string #587

Merged
merged 11 commits into from
Nov 21, 2022
Merged

[protobuf] - encode zipcode as a string #587

merged 11 commits into from
Nov 21, 2022

Conversation

puckpuck
Copy link
Contributor

@puckpuck puckpuck commented Nov 15, 2022

This changes the zipCode attribute of the Address message to be a string type instead of an int.

Also adds an attribute to the shipping service spans that contain the zipCode.

  • Appropriate CHANGELOG.md updated for non-trivial changes

@puckpuck puckpuck requested a review from a team November 15, 2022 23:47
@cartersocha
Copy link
Contributor

Could you add a change log item too?

Copy link
Member

@julianocosta89 julianocosta89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me, but when testing I got a weird behavior.
Even though everything was working when the requests were coming from the loadgen, when I tried to navigate to the page and "buy" a product, the shipping was 0,00 and I got the following error in the console:

shipping-service         | thread 'tokio-runtime-worker' panicked at 'called `Option::unwrap()` on a `None` value', src/shipping_service.rs:67:91
shipping-service         | 07:52:13 [INFO] GetQuoteRequest: Request { metadata: MetadataMap { headers: {"traceparent": "00-70c1bde2eddef2088aaceeefe54d15eb-460f45857d9e8886-01", "grpc-accept-encoding": "identity,deflate,gzip", "accept-encoding": "identity", "user-agent": "grpc-node-js/1.6.7", "content-type": "application/grpc", "te": "trailers"} }, message: GetQuoteRequest { address: None, items: [CartItem { product_id: "66VCHSJNUP", quantity: 1 }] }, extensions: Extensions }
frontend                 | Error: 1 CANCELLED: Call cancelled
frontend                 |     at Object.callErrorFromStatus (/app/node_modules/@grpc/grpc-js/build/src/call.js:31:26)
frontend                 |     at Object.onReceiveStatus (/app/node_modules/@grpc/grpc-js/build/src/client.js:189:52)
frontend                 |     at Object.onReceiveStatus (/app/node_modules/@grpc/grpc-js/build/src/client-interceptors.js:365:141)
frontend                 |     at Object.onReceiveStatus (/app/node_modules/@grpc/grpc-js/build/src/client-interceptors.js:328:181)
frontend                 |     at /app/node_modules/@grpc/grpc-js/build/src/call-stream.js:187:78
frontend                 |     at processTicksAndRejections (node:internal/process/task_queues:78:11) {
frontend                 |   code: 1,
frontend                 |   details: 'Call cancelled',
frontend                 |   metadata: Metadata { internalRepr: Map(0) {}, options: {} }
frontend                 | }

I'm not getting any error if I don't navigate via browser.

@puckpuck
Copy link
Contributor Author

@julianocosta89 can you try again? I made some changes to the frontend code to pass in the dummy address when it attempts to get the shipping quote.

@julianocosta89
Copy link
Member

@julianocosta89 can you try again? I made some changes to the frontend code to pass in the dummy address when it attempts to get the shipping quote.

Fixed! Good one!

@puckpuck puckpuck merged commit 133d089 into open-telemetry:main Nov 21, 2022
@puckpuck puckpuck deleted the string-zipcode branch November 21, 2022 13:44
jmichalak9 pushed a commit to jmichalak9/opentelemetry-demo that referenced this pull request Mar 22, 2024
* encode zipcode as a string

* encode zipcode as a string

* encode zipcode as a string

* add zip_code attribute

* update zipCode to string type

* update zipCode to string type

* add address to shipping api call

* add address to shipping api call

* zipcode as string
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.

4 participants