refactor(fetch): revert to original code in endSpanOnSuccess #3037

Ugzuzg · 2022-06-13T16:24:35Z

Which problem is this PR solving?

This spanResponse object is not used for anything, and I can't figure out its purpose:

opentelemetry-js/experimental/packages/opentelemetry-instrumentation-fetch/src/fetch.ts

Lines 324 to 334 in 0213d82

    
           const spanResponse = { 
        
             status: response.status, 
        
             statusText: response.statusText, 
        
             headers: response.headers, 
        
             url 
        
           }; 
        
           if (response.status >= 200 && response.status < 400) { 
        
             if (response.url != null && response.url !== '') { 
        
               spanResponse.url = url; 
        
             } 
        
           }

The override doesn't make sense, as it sets the same value already present in the initialised object:

opentelemetry-js/experimental/packages/opentelemetry-instrumentation-fetch/src/fetch.ts

Lines 330 to 334 in 0213d82

    
           if (response.status >= 200 && response.status < 400) { 
        
             if (response.url != null && response.url !== '') { 
        
               spanResponse.url = url; 
        
             } 
        
           }

I have amended the code to what I think makes sense and was the original intention. Opening a PR to have a discussion.

Short description of the changes

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Checklist:

Followed the style guidelines of this project
Unit tests have been added
Documentation has been updated

Ugzuzg · 2022-06-13T16:27:17Z

experimental/packages/opentelemetry-instrumentation-fetch/test/fetch.test.ts

+      );
+      assert.strictEqual(
+        attributes[keys[2]],
+        redirectedUrl,


Does it make sense that the http.url attribute's value is the url before the redirection, but http.host is the host after the redirection? Feels strange.

Hmm, you're right, it does indeed feel strange. As far as I can tell, it also does not follow the spec. There it states that:

Retries and redirects cause more than one physical HTTP request to be sent. A CLIENT span SHOULD be created for each one of these physical requests. No span is created corresponding to the "logical" (encompassing) request.

It looks like to me that we'd need to instrument fetch in a way that it creates spans on each redirect (in that case host.host and http.url should be the same). However, I have to admit that I do not know enough about the internals of fetch right now to come up with a way to implement that. We should create an issue to track this, though.

Since the discrepancy is introduced by this change, a better fix for now should be to just remove spanResponse variable, and then implement proper redirection spans.

Sounds good. If there's a way to create proper redirection spans we should do it 👍

Doesn't look like it is possible, sadly.
Redirects are not exposed to APIs: https://fetch.spec.whatwg.org/#atomic-http-redirect-handling
There's a manual redirect mode, but it can't be used in any useful way.

"manual"
Retrieves an opaque-redirect filtered response when a request is met with a redirect, to allow a service worker to replay the redirect offline. The response is otherwise indistinguishable from a network error, to not violate atomic HTTP redirect handling.

An opaque-redirect filtered response is a filtered response whose type is "opaqueredirect", status is 0, status message is the empty byte sequence, header list is empty, and body is null.

codecov · 2022-06-13T16:30:57Z

Codecov Report

Merging #3037 (e9d7830) into main (492a3e8) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3037      +/-   ##
==========================================
+ Coverage   93.09%   93.10%   +0.01%     
==========================================
  Files         195      195              
  Lines        6386     6384       -2     
  Branches     1349     1348       -1     
==========================================
- Hits         5945     5944       -1     
+ Misses        441      440       -1

Impacted Files	Coverage Δ
...s/opentelemetry-instrumentation-fetch/src/fetch.ts	`97.02% <100.00%> (+0.55%)`	⬆️

dyladan · 2022-06-17T18:08:33Z

@pichlermarc looks like you originally introduced this object in #2871 please look at this

pichlermarc · 2022-06-20T06:58:52Z

@Ugzuzg you're right, it does not make sense. I committed this by mistake in #2871. 😞 The code here is actually supposed to be the original code:

        function endSpanOnSuccess(span: api.Span, response: Response) {
          plugin._applyAttributesAfterFetch(span, options, response);
          if (response.status >= 200 && response.status < 400) {
            plugin._endSpan(span, spanData, response);
          } else {
            plugin._endSpan(span, spanData, {
              status: response.status,
              statusText: response.statusText,
              url,
            });
          }
        }

Back then, I must have been trying to figure out what caused the url to be null, but that was actually caused by #2884. After I figured out that this was the issue, I made the change to use a local implementation of parseUrl() and must have accidentally added these changes too.

I can create a fix, or you can do it with this PR.
Sorry about that and thanks for bringing this up! 😥

Ugzuzg · 2022-06-20T12:27:55Z

@pichlermarc, the current change in the pull request behaves in the same way you're describing (with the addition of response.url check). Do you still want me to change it? Or can we keep it as is?

pichlermarc · 2022-06-20T13:28:26Z

@Ugzuzg As it behaves the same I think we can keep this as is, thanks for working on this! 🙂
You'll just need a changelog entry and then we can get this PR going. 🙂

CHANGELOG.md

pichlermarc · 2022-06-20T14:35:26Z

experimental/packages/opentelemetry-instrumentation-fetch/test/fetch.test.ts

+      );
+      assert.strictEqual(
+        attributes[keys[2]],
+        redirectedUrl,


Hmm, you're right, it does indeed feel strange. As far as I can tell, it also does not follow the spec. There it states that:

Retries and redirects cause more than one physical HTTP request to be sent. A CLIENT span SHOULD be created for each one of these physical requests. No span is created corresponding to the "logical" (encompassing) request.

It looks like to me that we'd need to instrument fetch in a way that it creates spans on each redirect (in that case host.host and http.url should be the same). However, I have to admit that I do not know enough about the internals of fetch right now to come up with a way to implement that. We should create an issue to track this, though.

pichlermarc

Looks great! 🙂 Thanks for bringing this up and taking the time to fix it! 🙂

dyladan

Most of my comments have the same theme. The impl looks fine but the tests you added are very hard to read because of the indirect way you are accessing attribute values.

experimental/CHANGELOG.md

experimental/packages/opentelemetry-instrumentation-fetch/test/fetch.test.ts

legendecas · 2022-07-22T02:23:28Z

Nonblocking: an update to the PR title will be most helpful.

experimental/packages/opentelemetry-instrumentation-fetch/src/fetch.ts

dyladan · 2022-07-29T19:26:13Z

Not really a fix. I would retitle to refactor

dyladan · 2022-07-29T19:31:12Z

Not really a fix. I would retitle to refactor

I meant the PR title not so much the commit 😄

Ugzuzg requested a review from a team June 13, 2022 16:24

Ugzuzg commented Jun 13, 2022

View reviewed changes

Ugzuzg force-pushed the fix/fetch-span-response branch from c653bd4 to bd01667 Compare June 13, 2022 16:29

Ugzuzg force-pushed the fix/fetch-span-response branch 2 times, most recently from 63a498f to f9ee378 Compare June 13, 2022 16:32

Ugzuzg force-pushed the fix/fetch-span-response branch from f9ee378 to a635275 Compare June 20, 2022 12:29

Ugzuzg force-pushed the fix/fetch-span-response branch from a635275 to 077d9fa Compare June 20, 2022 13:40

pichlermarc reviewed Jun 20, 2022

View reviewed changes

Ugzuzg force-pushed the fix/fetch-span-response branch from 077d9fa to 229d902 Compare June 20, 2022 18:42

pichlermarc approved these changes Jun 21, 2022

View reviewed changes

dyladan reviewed Jun 21, 2022

View reviewed changes

Ugzuzg force-pushed the fix/fetch-span-response branch from 229d902 to d8fb4f7 Compare July 21, 2022 18:10

legendecas added the Skip Changelog label Jul 22, 2022

legendecas approved these changes Jul 22, 2022

View reviewed changes

Ugzuzg changed the title ~~fix(fetch): pass spanResponse to _endSpan~~ fix(fetch): remove unused spanResponse variable Jul 22, 2022

pichlermarc reviewed Jul 22, 2022

View reviewed changes

experimental/packages/opentelemetry-instrumentation-fetch/src/fetch.ts Outdated Show resolved Hide resolved

Ugzuzg force-pushed the fix/fetch-span-response branch 3 times, most recently from 47ab8c2 to de5270b Compare July 29, 2022 19:16

dyladan approved these changes Jul 29, 2022

View reviewed changes

experimental/packages/opentelemetry-instrumentation-fetch/src/fetch.ts Show resolved Hide resolved

refactor(fetch): revert to original code in endSpanOnSuccess

e9d7830

Ugzuzg force-pushed the fix/fetch-span-response branch from de5270b to e9d7830 Compare July 29, 2022 19:28

Ugzuzg changed the title ~~fix(fetch): remove unused spanResponse variable~~ refactor(fetch): revert to original code in endSpanOnSuccess Jul 29, 2022

vmarchaud approved these changes Jul 30, 2022

View reviewed changes

vmarchaud merged commit fc4dcba into open-telemetry:main Jul 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(fetch): revert to original code in endSpanOnSuccess #3037

refactor(fetch): revert to original code in endSpanOnSuccess #3037

Ugzuzg commented Jun 13, 2022 •

edited

Loading

Ugzuzg Jun 13, 2022 •

edited

Loading

pichlermarc Jun 20, 2022

Ugzuzg Jul 21, 2022

pichlermarc Jul 22, 2022

Ugzuzg Jul 29, 2022

codecov bot commented Jun 13, 2022 •

edited

Loading

dyladan commented Jun 17, 2022

pichlermarc commented Jun 20, 2022

Ugzuzg commented Jun 20, 2022

pichlermarc commented Jun 20, 2022

pichlermarc Jun 20, 2022

pichlermarc left a comment

dyladan left a comment

legendecas commented Jul 22, 2022

dyladan commented Jul 29, 2022

dyladan commented Jul 29, 2022

	const spanResponse = {
	status: response.status,
	statusText: response.statusText,
	headers: response.headers,
	url
	};
	if (response.status >= 200 && response.status < 400) {
	if (response.url != null && response.url !== '') {
	spanResponse.url = url;
	}
	}

refactor(fetch): revert to original code in endSpanOnSuccess #3037

refactor(fetch): revert to original code in endSpanOnSuccess #3037

Conversation

Ugzuzg commented Jun 13, 2022 • edited Loading

Which problem is this PR solving?

Short description of the changes

Type of change

How Has This Been Tested?

Checklist:

Ugzuzg Jun 13, 2022 • edited Loading

Choose a reason for hiding this comment

pichlermarc Jun 20, 2022

Choose a reason for hiding this comment

Ugzuzg Jul 21, 2022

Choose a reason for hiding this comment

pichlermarc Jul 22, 2022

Choose a reason for hiding this comment

Ugzuzg Jul 29, 2022

Choose a reason for hiding this comment

codecov bot commented Jun 13, 2022 • edited Loading

Codecov Report

dyladan commented Jun 17, 2022

pichlermarc commented Jun 20, 2022

Ugzuzg commented Jun 20, 2022

pichlermarc commented Jun 20, 2022

pichlermarc Jun 20, 2022

Choose a reason for hiding this comment

pichlermarc left a comment

Choose a reason for hiding this comment

dyladan left a comment

Choose a reason for hiding this comment

legendecas commented Jul 22, 2022

dyladan commented Jul 29, 2022

dyladan commented Jul 29, 2022

Ugzuzg commented Jun 13, 2022 •

edited

Loading

Ugzuzg Jun 13, 2022 •

edited

Loading

codecov bot commented Jun 13, 2022 •

edited

Loading