Fix JS HTTP template #611

bhautikpip · 2022-03-29T19:25:18Z

Description:

Bug:
original JS HTTP template has empty http response where as the retrieved trace would definitely have a non empty response.

Fix:

add Status http response fields in JS HTTP template to have parity with other languages template.

JS integration tests are failing since in expected template we are trying to compare against
[0].http.response={}, which is never present in retrieved trace

Link to tracking Issue:
https://github.com/aws-observability/aws-otel-js/runs/5041312760?check_suite_focus=true#step:8:182

Testing:

Documentation:

NathanielRN

It's not too important, but maybe you can find out why we needed this response before?

Can you also confirm that the X-Ray UI Console has the look and experience we expect?

That's why changing the template is kind of risky. Because although we can fix the tests, we need to check and make sure the backend isn't using this for something important on their end.

bhautikpip · 2022-03-29T19:45:07Z

It's not too important, but maybe you can find out why we needed this response before?

Can you also confirm that the X-Ray UI Console has the look and experience we expect?

That's why changing the template is kind of risky. Because although we can fix the tests, we need to check and make sure the backend isn't using this for something important on their end.

Response was always empty as shown in this PR. After we have added this template ADOT JS integration tests were never successful. Not sure what do you mean by X-Ray UI console has the look and experience? FYI, validation templates are only being used by test framework and not X-Ray backend. I think they are very safe to change to address integration testing needs. Thanks for approving this.

bhautikpip · 2022-03-29T20:07:38Z

@vasireddy99 can you take a look as well? we might need adot-sdk-maintainers group approval to merge this.

NathanielRN · 2022-03-29T20:07:50Z

Response was always empty as shown in this PR.

Sure but I think we should figure out why. The tests were passing with this empty dictionary before. I meant you should check OTel JS upstream to see if there were any changes to why this changed because we didn't change the template so upstream must have changed.

After we have added this template ADOT JS integration tests were never successful.

Are you sure? That PR was merged Dec 2020, but the tests succeeded 5 moths ago Nov 2021.

Not sure what do you mean by X-Ray UI console has the look and experience?

I mean the X-Ray UI for the Service Graph, Service Timeline, for a given trace. Check out this issue to learn how upstream changing they way they instrumented affected the X-Ray UI console experience. It degraded it.

FYI, validation templates are only being used by test framework and not X-Ray backend.

Yup they are not being used by the backend, but we set up these templates so that we can guarantee some attributes will arrive at the X-Ray backend if customers use OTel JS. It's our responsibility to make sure they are set, not the X-Ray backend's responsibility that's why they don't use it. If it breaks then it will break on the backend and they won't realize it. That's why these tests have to check it.

I think they are very safe to change to address integration testing needs. Thanks for approving this.

I agree it's a small change. But you can enhance the PR description by posting screenshots of the X-Ray UI to give confidence that this is the right change.

Aneurysm9 · 2022-03-29T20:40:48Z

I'm not sure this is the right change. It looks like the failing tests have more data in the http.response entry, not less:

validator_1  | 18:30:06.391 [main] INFO  com.amazon.aoc.validators.TraceValidator - value of retrieved trace map: 
{[0].name=aws-otel-integ-test, [0].id=03b955abb4882c0a, [0].start_time=1643826585.4815292, 
[0].trace_id=1-61facd99-60cb95af3eab1bd503352cba, [0].end_time=1643826585.5460968, 
[0].subsegments[0].name=aws.amazon.com:80, [0].subsegments[0].id=e926e1a91214e9f0, 
[0].subsegments[0].start_time=1643826585.519404, [0].subsegments[0].end_time=1643826585.568151,
[0].subsegments[0].namespace=remote, [0].subsegments[0].http.request.url=http://aws.amazon.com/, [0].subsegments[0].http.request.method=GET, [0].subsegments[0].http.request.client_ip=99.84.179.74,
[0].subsegments[0].http.response.status=301, [0].subsegments[0].http.response.content_length=0,
[0].subsegments[0].aws.xray.auto_instrumentation=false, [0].subsegments[0].aws.xray.sdk_version=0.25.0,
[0].subsegments[0].aws.xray.sdk=opentelemetry for nodejs, [0].subsegments[0].metadata.default[\"net.transport\"]=ip_tcp, [0].subsegments[0].metadata.default[\"http.flavor\"]=1.1,
[0].subsegments[0].metadata.default[\"http.response_content_length_uncompressed\"]=183,
[0].subsegments[0].metadata.default[\"http.status_text\"]=MOVED PERMANENTLY, [0].http.request.url=http://app:8080/outgoing-http-call, [0].http.request.method=GET, [0].http.request.user_agent=okhttp/4.9.0,
[0].http.request.client_ip=172.18.0.4, [0].http.response.status=200, [0].http.response.content_length=0,
[0].aws.xray.auto_instrumentation=false, [0].aws.xray.sdk_version=0.25.0, [0].aws.xray.sdk=opentelemetry for nodejs,
[0].metadata.default[\"otel.resource.telemetry.sdk.name\"]=opentelemetry, [0].metadata.default[\"net.transport\"]=ip_tcp,
[0].metadata.default[\"http.flavor\"]=1.1, [0].metadata.default[\"http.route\"]=/outgoing-http-call,
[0].metadata.default[\"http.status_text\"]=OK, [0].metadata.default[\"otel.resource.service.name\"]=aws-otel-integ-test,
[0].metadata.default[\"otel.resource.telemetry.sdk.language\"]=nodejs, [0].metadata.default[\"net.host.ip\"]=172.18.0.3,
[0].metadata.default[\"otel.resource.telemetry.sdk.version\"]=0.25.0, [1].name=aws.amazon.com:80,
[1].id=22dc8f0f053c707f, [1].parent_id=e926e1a91214e9f0, [1].start_time=1643826585.519404,
[1].trace_id=1-61facd99-60cb95af3eab1bd503352cba, [1].end_time=1643826585.568151, [1].inferred=true,
[1].http.request.url=http://aws.amazon.com/, [1].http.request.method=GET, [1].http.request.client_ip=99.84.179.74,
[1].http.response.status=301, [1].http.response.content_length=0}
validator_1  | 18:30:06.393 [main] ERROR com.amazon.aoc.validators.TraceValidator - mis target data: [0].http.response

bhautikpip · 2022-03-29T21:18:02Z

I'm not sure this is the right change. It looks like the failing tests have more data in the http.response entry, not less:

validator_1  | 18:30:06.391 [main] INFO  com.amazon.aoc.validators.TraceValidator - value of retrieved trace map: 
{[0].name=aws-otel-integ-test, [0].id=03b955abb4882c0a, [0].start_time=1643826585.4815292, 
[0].trace_id=1-61facd99-60cb95af3eab1bd503352cba, [0].end_time=1643826585.5460968, 
[0].subsegments[0].name=aws.amazon.com:80, [0].subsegments[0].id=e926e1a91214e9f0, 
[0].subsegments[0].start_time=1643826585.519404, [0].subsegments[0].end_time=1643826585.568151,
[0].subsegments[0].namespace=remote, [0].subsegments[0].http.request.url=http://aws.amazon.com/, [0].subsegments[0].http.request.method=GET, [0].subsegments[0].http.request.client_ip=99.84.179.74,
[0].subsegments[0].http.response.status=301, [0].subsegments[0].http.response.content_length=0,
[0].subsegments[0].aws.xray.auto_instrumentation=false, [0].subsegments[0].aws.xray.sdk_version=0.25.0,
[0].subsegments[0].aws.xray.sdk=opentelemetry for nodejs, [0].subsegments[0].metadata.default[\"net.transport\"]=ip_tcp, [0].subsegments[0].metadata.default[\"http.flavor\"]=1.1,
[0].subsegments[0].metadata.default[\"http.response_content_length_uncompressed\"]=183,
[0].subsegments[0].metadata.default[\"http.status_text\"]=MOVED PERMANENTLY, [0].http.request.url=http://app:8080/outgoing-http-call, [0].http.request.method=GET, [0].http.request.user_agent=okhttp/4.9.0,
[0].http.request.client_ip=172.18.0.4, [0].http.response.status=200, [0].http.response.content_length=0,
[0].aws.xray.auto_instrumentation=false, [0].aws.xray.sdk_version=0.25.0, [0].aws.xray.sdk=opentelemetry for nodejs,
[0].metadata.default[\"otel.resource.telemetry.sdk.name\"]=opentelemetry, [0].metadata.default[\"net.transport\"]=ip_tcp,
[0].metadata.default[\"http.flavor\"]=1.1, [0].metadata.default[\"http.route\"]=/outgoing-http-call,
[0].metadata.default[\"http.status_text\"]=OK, [0].metadata.default[\"otel.resource.service.name\"]=aws-otel-integ-test,
[0].metadata.default[\"otel.resource.telemetry.sdk.language\"]=nodejs, [0].metadata.default[\"net.host.ip\"]=172.18.0.3,
[0].metadata.default[\"otel.resource.telemetry.sdk.version\"]=0.25.0, [1].name=aws.amazon.com:80,
[1].id=22dc8f0f053c707f, [1].parent_id=e926e1a91214e9f0, [1].start_time=1643826585.519404,
[1].trace_id=1-61facd99-60cb95af3eab1bd503352cba, [1].end_time=1643826585.568151, [1].inferred=true,
[1].http.request.url=http://aws.amazon.com/, [1].http.request.method=GET, [1].http.request.client_ip=99.84.179.74,
[1].http.response.status=301, [1].http.response.content_length=0}
validator_1  | 18:30:06.393 [main] ERROR com.amazon.aoc.validators.TraceValidator - mis target data: [0].http.response

Yeah, the upstream probably started populating response fields so we should definitely add some of the fields like Status to have the parity with other languages. I have made the change. I'm also planning to remove this template altogether to use the default template so that we don't have maintain templates for all the languages.

bhautikpip · 2022-03-29T22:14:13Z

@NathanielRN

Yep. I think you're right I was confusing years. I have made the change to add Status in the response and also planning to remove the template itself to use the default one. Thanks for suggesting this!

bhautikpip · 2022-03-29T23:59:28Z

Opened the issue to use only default template: #613. I don't want to add this change in this PR for better tracking and visibility. Also, I want to make sure first that this change really fixes JS integration test failures before making other enhancements.

Aneurysm9 · 2022-03-30T15:32:51Z

validator/src/main/resources/expected-data-template/js/jsAppExpectedHTTPTrace.mustache

@@ -21,6 +22,7 @@
          "method": "GET"
        },
        "response": {
+          "status": 200


Is this correct? Doesn't this get a 301 to https://...?

yep. this should be 301. Just tested the sample app locally.

{ "Id": "1-62448ed9-4733ff1a38db832ba446bbb7", "Duration": 0.075, "LimitExceeded": false, "Segments": [ { "Id": "20130a68ea1b0408", "Document": { "id": "20130a68ea1b0408", "name": "aws-otel-integ-test", "start_time": 1648660185.9195466, "trace_id": "1-62448ed9-4733ff1a38db832ba446bbb7", "end_time": 1648660185.990404, "fault": false, "error": false, "throttle": false, "http": { "request": { "url": "http://localhost:8080/outgoing-http-call", "method": "GET", "user_agent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/99.0.4844.84 Safari/537.36", "client_ip": "127.0.0.1" }, "response": { "status": 200, "content_length": 0 } }, "aws": { "xray": { "auto_instrumentation": false, "sdk_version": "0.25.0", "sdk": "opentelemetry for nodejs" } }, "metadata": { "default": { "otel.resource.telemetry.sdk.name": "opentelemetry", "net.transport": "ip_tcp", "http.flavor": "1.1", "http.route": "/outgoing-http-call", "http.status_text": "OK", "otel.resource.service.name": "aws-otel-integ-test", "otel.resource.telemetry.sdk.language": "nodejs", "net.host.ip": "127.0.0.1", "otel.resource.telemetry.sdk.version": "0.25.0" } }, "subsegments": [ { "id": "584849890004c9f8", "name": "aws.amazon.com:80", "start_time": 1648660185.9197578, "end_time": 1648660185.9947398, "fault": false, "error": false, "throttle": false, "http": { "request": { "url": "http://aws.amazon.com/", "method": "GET", "client_ip": "18.65.225.66" }, "response": { "status": 301, "content_length": 0 } }, "aws": { "xray": { "auto_instrumentation": false, "sdk_version": "0.25.0", "sdk": "opentelemetry for nodejs" } }, "metadata": { "default": { "net.transport": "ip_tcp", "http.flavor": "1.1", "http.response_content_length_uncompressed": 183, "http.status_text": "MOVED PERMANENTLY" } }, "namespace": "remote" } ] } }, { "Id": "0d16cdad11ac4230", "Document": { "id": "0d16cdad11ac4230", "name": "aws.amazon.com:80", "start_time": 1648660185.9197578, "trace_id": "1-62448ed9-4733ff1a38db832ba446bbb7", "end_time": 1648660185.9947398, "parent_id": "584849890004c9f8", "inferred": true, "http": { "request": { "url": "http://aws.amazon.com/", "method": "GET", "client_ip": "18.65.225.66" }, "response": { "status": 301, "content_length": 0 } } } } ] }

I suppose this raises the question of why there isn't a segment for following the redirect, but that can be investigated separately from this PR.

bhautikpip · 2022-03-30T18:21:31Z

@Aneurysm9 can you also merge this PR? I don't think I have permissions to merge this PR.

Fix JS HTTP template

b832568

bhautikpip requested a review from a team as a code owner March 29, 2022 19:25

NathanielRN approved these changes Mar 29, 2022

View reviewed changes

bhautikpip changed the title ~~Fix JS HTTP template~~ Bug Fix JS HTTP template Mar 29, 2022

bhautikpip changed the title ~~Bug Fix JS HTTP template~~ Fix JS HTTP template Mar 29, 2022

Added status field in response data structure

9b87107

Aneurysm9 reviewed Mar 30, 2022

View reviewed changes

modified the subsegment status code

ca7a938

bhautikpip requested a review from Aneurysm9 March 30, 2022 17:23

Aneurysm9 approved these changes Mar 30, 2022

View reviewed changes

bryan-aguilar merged commit 0b6435a into aws-observability:terraform Mar 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix JS HTTP template #611

Fix JS HTTP template #611

bhautikpip commented Mar 29, 2022 •

edited

Loading

NathanielRN left a comment

bhautikpip commented Mar 29, 2022 •

edited

Loading

bhautikpip commented Mar 29, 2022

NathanielRN commented Mar 29, 2022

Aneurysm9 commented Mar 29, 2022

bhautikpip commented Mar 29, 2022 •

edited

Loading

bhautikpip commented Mar 29, 2022 •

edited

Loading

bhautikpip commented Mar 29, 2022 •

edited

Loading

Aneurysm9 Mar 30, 2022

bhautikpip Mar 30, 2022

Aneurysm9 Mar 30, 2022

bhautikpip commented Mar 30, 2022

Fix JS HTTP template #611

Fix JS HTTP template #611

Conversation

bhautikpip commented Mar 29, 2022 • edited Loading

NathanielRN left a comment

Choose a reason for hiding this comment

bhautikpip commented Mar 29, 2022 • edited Loading

bhautikpip commented Mar 29, 2022

NathanielRN commented Mar 29, 2022

Aneurysm9 commented Mar 29, 2022

bhautikpip commented Mar 29, 2022 • edited Loading

bhautikpip commented Mar 29, 2022 • edited Loading

bhautikpip commented Mar 29, 2022 • edited Loading

Aneurysm9 Mar 30, 2022

Choose a reason for hiding this comment

bhautikpip Mar 30, 2022

Choose a reason for hiding this comment

Aneurysm9 Mar 30, 2022

Choose a reason for hiding this comment

bhautikpip commented Mar 30, 2022

bhautikpip commented Mar 29, 2022 •

edited

Loading

bhautikpip commented Mar 29, 2022 •

edited

Loading

bhautikpip commented Mar 29, 2022 •

edited

Loading

bhautikpip commented Mar 29, 2022 •

edited

Loading

bhautikpip commented Mar 29, 2022 •

edited

Loading