fix(instrumentation-fetch, instrumentation-xml-http-request) content length attributes now prese

[arriIsHere]

Which problem is this PR solving?

http.response_content_length and http.response_content_length_uncompressed attributes are not present on network spans from instrumentation-xml-http-request and instrumentation-fetch plugins when the ignoreNetworkEvents configuration property is set to true

This PR changes addSpanNetworkEvents to include the options to skip over adding network events but still include the previously mentioned properties.

Fixes #5229

Short description of the changes

By skipping over the call to addSpanNetworkEvents the mentioned attributes were not getting added to the main and preflight network spans. We move the logic from the calls to the method into the addSpanNetworkEvents call itself with a third boolean parameter. This will disable adding network events spans while still adding the attributes.

While fixing this we considered several different changes, we went with adding an optional paramiter to addSpanNetworkEvents

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• 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?

Automated unit tests were written as well as testing with the fetch and xhr examples to verify the attributes are being added with the proposed code.

1. Open the examples/opentelemetry-web/examples/fetchexample
2. Start example
3. Note that the GET event has a http.response_content_length attribute
4. Add the ignoreNetworkEvents: true config to the FetchInstrumentation in the index.js file
5. Run example
6. The http.response_content_length attribute should still be present on the GET span

Checklist:

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

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: arriIsHere / name: Arriana Blais (f9ed04d, bd5e6e3, d645e6c, 679a84e, 9fbf814, 6b1c370, e03f44e, 584febf, 4c571bf, 12d20c3, 228dc42, e1d56d3, 43f5627)

[arriIsHere]

Other options explored

We explored several other options in trying to solve this issue. Exposing a new helper method, refactoring the content length attribute code to fetch and http and our preferred method one where we passed a flag down through a parameter.

New Helper method

In this option we exposed a new helper method to handle the content length attributes. This would then be called from both the addSpanNetworkEvents as well as the network instrumentation libraries to ensure these attributes were added.

/**
 * Helper function for adding content length attributes to a network span
 * @param span 
 * @param resource 
 */
export function addSpanContentLengthAttributes(
  span: api.Span, 
  resource: PerformanceEntries
): void {
  const encodedLength = resource[PTN.ENCODED_BODY_SIZE];
  if (encodedLength !== undefined) {
    span.setAttribute(SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH, encodedLength);
  }
  const decodedLength = resource[PTN.DECODED_BODY_SIZE];
  // Spec: Not set if transport encoding not used (in which case encoded and decoded sizes match)
  if (decodedLength !== undefined && encodedLength !== decodedLength) {
    span.setAttribute(
      SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH_UNCOMPRESSED,
      decodedLength
    );
  }
}

This would be called as such inside the fetch and xhr libraries:

  if (!this.getConfig().ignoreNetworkEvents) {
    addSpanNetworkEvents(childSpan, corsPreFlightRequest);
  } else {
    addSpanContentLenthAttributes(childSpan, corsPreFlightRequest);
  }

Pros:

• Maintains compatibility with any 3rd party OTel tools that use addSpanNetworkEvents
• Moves logic into own function

Cons:

• Adds another exported method to the trace web utils
• addSpanNetworkEvents still has the side effect of adding content length attributes

Moving logic into libraries

This involved moving the logic that adds content length attributes into the fetch and xhr instrumentation libraries and removing it from the addSpanNetworkEvents this would be implimented similar to this:

private _findResourceAndAddNetworkEvents(
  xhrMem: XhrMem,
  span: api.Span,
  spanUrl?: string,
  startTime?: api.HrTime,
  endTime?: api.HrTime
): void {
     //...
      if (!this.getConfig().ignoreNetworkEvents) {
        addSpanNetworkEvents(span, mainRequest);
      }
      this._addSpanContentLenthAttributes(span, mainRequest);
    }
  }

  /**
   * Add content length attributes to a network span
   * @param span 
   * @param resource 
   */
private _addSpanContentLenthAttributes(
    span: api.Span, 
    resource: PerformanceEntries
): void {
    const encodedLength = resource[PTN.ENCODED_BODY_SIZE];
    if (encodedLength !== undefined) {
      span.setAttribute(SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH, encodedLength);
    }
    const decodedLength = resource[PTN.DECODED_BODY_SIZE];
    // Spec: Not set if transport encoding not used (in which case encoded and decoded sizes match)
    if (decodedLength !== undefined && encodedLength !== decodedLength) {
      span.setAttribute(
        SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH_UNCOMPRESSED,
        decodedLength
      );
    }
}

Pros:

• Moves logic completely out of addSpanNetworkEvents
• Does not add any new public util methods

Cons:

• Would be a breaking change for any 3rd party plugins that expect addSpanNetworkEvents to add content length attributes.

Adding optional parameter to disable network events (Chosen)

We move the logic from the calls to the method into the addSpanNetworkEvents call itself with a third boolean parameter. This will disable adding network events spans while still adding the attributes.

Pros:

• Maintains backwards compatability for any 3rd party plugins that expect addSpanNetworkEvents to add content length.
• Does not add any new plublic util methods.

Cons:

• addSpanNetworkEvents still has the side effect of adding content length attributes.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.65%. Comparing base (484af40) to head (43f5627).
Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5230      +/-   ##
==========================================
+ Coverage   94.60%   94.65%   +0.05%     
==========================================
  Files         315      315              
  Lines        8011     8012       +1     
  Branches     1617     1617              
==========================================
+ Hits         7579     7584       +5     
+ Misses        432      428       -4     

see 5 files with indirect coverage changes

[pichlermarc]

thanks for fixing this 🙂

[pichlermarc]

ah, that's the wrong changelog - it should go into CHANGELOG.md in the repository root 🙂

[pichlermarc]

hmm, that's really unfortunate.

I'd really like to avoid carrying this forward to 2.0 - @arriIsHere would you mind opening a follow-up PR towards the next branch later to implement option 1 "new helper method" from this comment there? It adds one more function but we'll be able to get rid of the body-size side-effect in this function, which I think cleans it up a bit.

I plans to move @opentelemetry/instrumentation-xhr and @opentelemetry/instrumentation-fetch into the same package as a follow up, which may even let us drop these functions from the public API of @opentelemetry/sdk-trace-web since they're not really SDK functions. That'd also go a long way of discouraging the anti-pattern of direct depending on the SDK for instrumentation.

[arriIsHere]

Absolutely! I can finish that up this morning.

[pichlermarc]

awesome, thank you 🙂

[arriIsHere]

@pichlermarc opened #5257 against next